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

page with numeric title generates numeric alias #1598

Closed
fritzmg opened this issue Jul 4, 2018 · 13 comments
Closed

page with numeric title generates numeric alias #1598

fritzmg opened this issue Jul 4, 2018 · 13 comments
Assignees
Labels
Milestone

Comments

@fritzmg
Copy link
Contributor

fritzmg commented Jul 4, 2018

If you create a page with a title that only contains numbers, the auto-generated alias will also only contain those numbers. However, you will then be unable to access that page in the frontend.

Reproduction in the Contao online demo:

  1. Create a new regular page within the English website root.
  2. Set its title to 1 for example.
  3. Set the page to published.
  4. Save the page - the alias will also be 1.
  5. Try to open https://demo.contao.org/en/1

This will redirect you to https://demo.contao.org/en/ - since Contao could not find that page. You can repeat these steps with any number.

In Contao 3, such automatically generated numeric aliases are prepended with id-.

@leofeyer
Copy link
Member

leofeyer commented Jul 4, 2018

Does this affect Contao 4.4 or just 4.5?

@m-vo
Copy link
Member

m-vo commented Jul 4, 2018

I can confirm this at least for 4.5. It doesn't make any difference wether the alias is generated or put in manually.

Btw.: 404 is a working alias for 404 pages :)

(Possibly related? #1508 )

@leofeyer
Copy link
Member

leofeyer commented Jul 4, 2018

I can confirm this at least for 4.5.

This does not help unfortunately. We switched to the slug generator in Contao 4.5, so I have to know whether this issue occurs in Contao 4.4 as well.

@xchs
Copy link
Contributor

xchs commented Jul 4, 2018

so I have to know whether this issue occurs in Contao 4.4 as well.

It doesn't.

@fritzmg
Copy link
Contributor Author

fritzmg commented Jul 4, 2018

I can confirm that this issues does not occur in Contao 4.4. In Contao 4.4 the alias will also automatically be prepended with id-.

@m-vo
Copy link
Member

m-vo commented Jul 4, 2018

Sorry if this is obvious: What was the reason to disallow numeric aliases in the first place?

@fritzmg
Copy link
Contributor Author

fritzmg commented Jul 4, 2018

I am guessing this is related to the decision to no allow pages to be accessible via their ID.

@leofeyer leofeyer added the bug label Jul 4, 2018
@leofeyer leofeyer added this to the 4.5.11 milestone Jul 4, 2018
@asaage
Copy link

asaage commented Jul 4, 2018

image

So should numeric aliases be allowed but not be interpreted as Page-id's?

@fritzmg
Copy link
Contributor Author

fritzmg commented Jul 5, 2018

@asaage I think this cannot be done without breaking backwards compatibility.

For instance, we could change this line

$pageModel = \PageModel::findPublishedByIdOrAlias($pageId);

in the FrontendIndex to

$pageModel = \PageModel::findByAliases([$pageId]);

(or a new findPublishedByAlias method could be implemented in the PageModel)

However, people who are using the getPageIdFromUrl hook might return a page ID instead of an alias and this would break their code.

@m-vo
Copy link
Member

m-vo commented Jul 5, 2018

Yeah.. methods like findPublishedByIdOrAlias() are a problem and as long they are ambigous, there is nothing we can do that won't break. I wonder if there is any place in the codebase where this mixture of loose types and magic is actually better suited than two explicit methods...

@m-vo
Copy link
Member

m-vo commented Jul 5, 2018

Page alias generation does only handle duplicates...

$arrCheck = Config::get('addLanguageToUrl') ? $arrPages[$strDomain][$strLanguage] : $arrPages[$strDomain];
// Check if there are multiple results for the current domain
if (!empty($arrCheck))
{
if (!$autoAlias)
{
throw new Exception(sprintf($GLOBALS['TL_LANG']['ERR']['aliasExists'], $varValue));
}
$varValue .= '-' . $dc->id;
}

($arrCheck is defined out of scope by the way)

... so checking for numeric only values got missing I suppose?

@leofeyer leofeyer removed this from the 4.5.11 milestone Jul 17, 2018
@leofeyer
Copy link
Member

As discussed on the developer's meeting, we have to re-add the id- prefix as there is no backwards compatible way to change the current logic (e.g. insert tags mostly use IDs instead of aliases).

@leofeyer leofeyer added this to the 4.5.11 milestone Jul 24, 2018
@leofeyer leofeyer self-assigned this Aug 13, 2018
leofeyer added a commit that referenced this issue Aug 13, 2018
@leofeyer
Copy link
Member

Fixed in b888454.

leofeyer added a commit to contao/calendar-bundle that referenced this issue Aug 13, 2018
leofeyer added a commit to contao/faq-bundle that referenced this issue Aug 13, 2018
leofeyer added a commit to contao/news-bundle that referenced this issue Aug 13, 2018
leofeyer added a commit to contao/newsletter-bundle that referenced this issue Aug 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants