Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] do not require pid for numeric article ids #201

Closed
wants to merge 5 commits into from

Conversation

Projects
None yet
4 participants
@fritzmg
Copy link
Contributor

commented Nov 26, 2018

During refactoring of one of my extensions I noticed the following: currently is impossible to do something as simple as:

<?= \Contao\Controller::getArticle(2) ?>

This code only works on the page to which the article with ID 2 belongs to.

Why? Controller::getArticle uses

/** @var PageModel $objPage */
global $objPage;
if (\is_object($varId))
{
$objRow = $varId;
}
else
{
if (!$varId)
{
return '';
}
$objRow = \ArticleModel::findByIdOrAliasAndPid($varId, (!$blnIsInsertTag ? $objPage->id : null));
if ($objRow === null)
{
return false;
}
}

And ArticleModel::findByIdOrAliasAndPid will always add the pid to the query, when it is present:

public static function findByIdOrAliasAndPid($varId, $intPid, array $arrOptions=array())
{
$t = static::$strTable;
$arrColumns = !is_numeric($varId) ? array("$t.alias=?") : array("$t.id=?");
$arrValues = array($varId);
if ($intPid)
{
$arrColumns[] = "$t.pid=?";
$arrValues[] = $intPid;
}
return static::findOneBy($arrColumns, $arrValues, $arrOptions);
}

Since Controller::getArticle only passes null for $intPid when it is used as an insert tag and otherwise always passes the ID of the current page, it is impossible to retreive the full markup of an article (including its wrapper) on any other page via Controller::getArticle.

This seems like an unecessary limitation. If you have the numeric ID of an article, the parent ID does not matter at all. In my extension I work around that problem by doing something like this:

// Override global page object
global $objPage;
$currentPage = $objPage;
$article = $articleModel->findById($articleId);
$objPage = $pageModel->findById($article->pid);

// Get the fully rendered article
$renderedArticle = $controller->getArticle($articleId, false, false, $column);

// Reset global page object
$objPage = $currentPage;

This PR would fix the limitation directly within ArticleModel::findByIdOrAliasAndPid. However we could also do

-   $objRow = \ArticleModel::findByIdOrAliasAndPid($varId, (!$blnIsInsertTag ? $objPage->id : null));
+   $objRow = \ArticleModel::findByIdOrAliasAndPid($varId, (!$blnIsInsertTag && !\is_numeric($varId) ? $objPage->id : null));

within Controller::getArticle. Though I think the former makes more sense since, as I mentioned before, the pid does not matter, if you have the numeric ID of an article.

Besides, I interpret the meaning behind the function name findByIdOrAliasAndPid as:

find by id || alias && pid

i.e., find by either an ID … or an alias and a PID.

@leofeyer

This comment has been minimized.

Copy link
Member

commented Dec 3, 2018

The logic is (id || alias) && pid though and not id || (alias && pid), therefore we should do something like

if (is_numeric($varId)) {
    $row = ArticleModel::findByPk($varId);
} else {
    $row = ArticleModel::findByIdOrAliasAndPid($varId, (!$blnIsInsertTag ? $objPage->id : null));
}
@fritzmg

This comment has been minimized.

Copy link
Contributor Author

commented Dec 3, 2018

The logic is (id || alias) && pid

I know, that's the current state. But you do not need a pid for an id - the id is unique across all tl_article entries. You only need the pid when using the alias, since there can be multiple tl_article entries for the same alias (I think? In multidomain installations?). Or what would be the purpose of querying the database for an additional parent ID, if you already have the ID?

@leofeyer

This comment has been minimized.

Copy link
Member

commented Dec 3, 2018

purpose of querying the database for an additional parent ID, if you already have the ID?

There is none, because the ID is unique. But if we decide to change this, we would have to change it everywhere and not just in one single method. And there is no guarantee that it does not imply a BC break.

@fritzmg

This comment has been minimized.

Copy link
Contributor Author

commented Dec 3, 2018

True, it would imply a BC break... someone could rely on there not being a result, if the pid does not fit the alias or id 😁

@fritzmg

This comment has been minimized.

Copy link
Contributor Author

commented Dec 3, 2018

Two methods would be affected. ArticleModel::findByIdOrAliasAndPid and ArticleModel::findPublishedByIdOrAliasAndPid. No other Contao Core model has an "id or alias and pid" method.

@leofeyer leofeyer force-pushed the contao:master branch 2 times, most recently from cb069f4 to 10ba742 Dec 4, 2018

@leofeyer

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

I would not risk a BC break. @contao/developers WDYT?

@ausi

This comment has been minimized.

Copy link
Member

commented Dec 10, 2018

Could we add a new method to the model, use that one in Controller::getArticle() and deprecate the old one?

@fritzmg

This comment has been minimized.

Copy link
Contributor Author

commented Dec 11, 2018

The BC break would still apply to Controller::getArticle().

@aschempp

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2018

The best backwards compatible option I can think of is something like allow null for $blnIsInsertTag and add a the model query to ($blnIsInsertTag === false ? $objPage->id : null)

@leofeyer leofeyer force-pushed the contao:master branch 3 times, most recently from ce9e981 to 8b4b00d Feb 18, 2019

@leofeyer leofeyer removed the defect label Mar 13, 2019

@leofeyer leofeyer force-pushed the contao:master branch from 804d2ed to c0fc631 Mar 27, 2019

@aschempp

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

@fritzmg will you update the PR according to my comment?

@fritzmg

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

I will, yes :)

@leofeyer leofeyer force-pushed the contao:master branch 2 times, most recently from 6c52109 to 03f6899 Jun 5, 2019

@fritzmg

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

I have pushed the changes. Doing something like

<?= \Contao\Controller::getArticle(2, false, null) ?>

will give you the full HTML markup of the article with ID 2, independent from the current $objPage object.

@leofeyer leofeyer added feature and removed up for discussion labels Jun 6, 2019

@leofeyer leofeyer added this to the 4.8 milestone Jun 6, 2019

@leofeyer leofeyer force-pushed the contao:master branch from 67bdc5c to d42ccf4 Jun 13, 2019

@leofeyer

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

will give you the full HTML markup of the article with ID 2

According to your phpDoc comment, it will give you the article with "no HTML markup for the container". What is correct?

And what if I want an article independent from the current page but with the full HTML markup?

@fritzmg

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

According to your phpDoc comment, it will give you the article with "no HTML markup for the container". What is correct?

"no HTML markup for the container" only applies when the third parameter $blnIsInsertTag is true. In all other cases the HTML markup is not omitted.

@leofeyer

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

I see. However, that is really ugly. Why did you not add a fourth argument?

@fritzmg

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

You mean a fifth, like $blnIgnorePageRelation and

($blnIsInsertTag === false && !$blnIgnorePageRelation ? $objPage->id : null)

which would lead to

<?= \Contao\Controller::getArticle(2, false, false, 'main', true) ?>

? Sure, I don't really care. Imho this is just as ugly 😁. Personally i'd still rather change the ArticleModel functions, because getting the fulll markup of an article should ideally be as easy as

<?= \Contao\Controller::getArticle(2) ?>

But we cannot do that without breaking BC.

@leofeyer

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

Let's discuss the implementation in the next Mumble call.

@leofeyer

This comment has been minimized.

Copy link
Member

commented Jul 4, 2019

As discussed in Mumble on July 4th, we want to add a fifth argument to the existing method and a new method getArticleById() which calls getArticle() with the correct arguments.

@fritzmg

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

Sounds good, I'll adjust the PR accordingly.

*
* @return string|boolean The article HTML markup or false
*/
public static function getArticleById($intId, $blnMultiMode=false, $blnIsInsertTag=false, $strColumn='main')

This comment has been minimized.

Copy link
@ausi

ausi Jul 4, 2019

Member

I think only the $intId parameter should be used here, without the other three parameters.

This comment has been minimized.

Copy link
@fritzmg

fritzmg Jul 4, 2019

Author Contributor

But then you will not be able to control whether you want the container markup. The $strColumn variable on the other hand seems to be a left over from the past. It is passed to the ModuleArticle instance which in turn passes it to the template. But the current default mod_article template does not use this variable.

This comment has been minimized.

Copy link
@leofeyer

leofeyer Jul 4, 2019

Member

Then why did you add it to the new method?

This comment has been minimized.

Copy link
@leofeyer

leofeyer Jul 4, 2019

Member

Ah, it is used here:

$arrElements[] = $this->getContentElement($objRow, $this->strColumn);

This comment has been minimized.

Copy link
@fritzmg

fritzmg Jul 4, 2019

Author Contributor

Then why did you add it to the new method?

I only looked at it when @ausi questioned the addition of all parameters ;)

Ah, it is used here:

Hm, same story there though. It is passed to the template, but not used by default anywhere. But some people might have custom content element template and check on the column.

This comment has been minimized.

Copy link
@leofeyer

leofeyer Jul 10, 2019

Member

I think you have to search for "inColumn" to find its usage, e.g. here:

if ($objArticle->inColumn != 'main')
{
$link .= $objArticle->inColumn . ':';
}

@leofeyer
Copy link
Member

left a comment

Since it seems that we cannot get rid of the method arguments, I vote for removing the new getArticleById() method again. A fifth argument is just as ugly IMHO. 😄

@fritzmg

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

I was able to solve my original problem in another way - since the getArticle method also accepts a complete ArticleModel object which completely circumvents the issue (see fritzmg/contao-inherit-article@9804891).

So as a workaround you can also do the following:

<?= \Contao\Controller::getArticle(\Contao\ArticleModel::findById(2)) ?>

Which I find at least better than ugly additional parameters ;). Thus currently I would vote for: leave everything as it is & change everything in Contao 5, if still applicable …

@leofeyer

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

That seems to be the best solution indeed. 😄

@leofeyer leofeyer closed this Jul 10, 2019

@leofeyer leofeyer removed this from the 4.8 milestone Jul 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.