contao / core-bundle

[READ-ONLY] Contao Core Bundle
GNU Lesser General Public License v3.0
122 stars 57 forks source link

page with numeric title generates numeric alias #1598

Closed fritzmg closed 6 years ago

fritzmg commented 6 years ago

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 commented 6 years ago

Does this affect Contao 4.4 or just 4.5?

m-vo commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

It doesn't.

fritzmg commented 6 years ago

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 commented 6 years ago

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

fritzmg commented 6 years ago

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

asaage commented 6 years ago

image

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

fritzmg commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

Page alias generation does only handle duplicates... https://github.com/contao/core-bundle/blob/21f90f74431ee23c9996fca683c7bb1ee7e75e66/src/Resources/contao/dca/tl_page.php#L1099-L1110 ($arrCheck is defined out of scope by the way)

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

leofeyer commented 6 years ago

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 commented 6 years ago

Fixed in b888454ffbcafbb42ee5756e429a11b0d0079186.