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 title starts with a number #707

Closed
netzarbeiter opened this issue Feb 15, 2017 · 30 comments
Closed

Page title starts with a number #707

netzarbeiter opened this issue Feb 15, 2017 · 30 comments
Assignees
Labels
Milestone

Comments

@netzarbeiter
Copy link
Member

When the page title starts with a number, the page alias gets an id in front.
e. g. page title = 1. Contao Konferenz / page alias = id-1-contao-konferenz
Is this behaviour still required or can we left out the id?

@Toflar
Copy link
Member

Toflar commented Feb 15, 2017

I think this is a leftover from our long dropped database abstraction layer. I don't see a reason why this should be needed for a page alias and I'm okay with dropping it in 4.4.

@leofeyer
Copy link
Member

leofeyer commented Feb 17, 2017

I believe that the prefix is added by the standardize() method because HTML IDs must not start with a numeric character. I don't recall any DBAL related issues. @Toflar Do you probably mean this?

https://github.com/contao/core-bundle/blob/master/src/Resources/contao/library/Contao/Model.php#L829

@Toflar
Copy link
Member

Toflar commented Feb 17, 2017

No I didn't specifically mean anything like this. I just thought there was something like that veeeeery long time ago ;-)
But well, the alias is not used for HTML IDs so I'm very much okay with removing the id- prefix in the generateAlias() method.

@leofeyer
Copy link
Member

Which we obviously cannot do without breaking BC.

@Toflar
Copy link
Member

Toflar commented Feb 17, 2017

I can't see any BC here. It's just a page alias, page aliases only change when you edit the page. And honestly, I don't know any reason why one would like to have the id- prefix. And still, if you like it you can re-add it manually :)

@lionel-m
Copy link

I believe that the prefix is added by the standardize() method because HTML IDs must not start with a numeric character.

Yes but since HTML5, it's allowed. (http://stackoverflow.com/a/36458675)

@leofeyer leofeyer added the bug label Feb 17, 2017
@leofeyer leofeyer added this to the 4.3.6 milestone Feb 17, 2017
@leofeyer
Copy link
Member

I can't see any BC here. It's just a page alias, page aliases only change when you edit the page.

The standardize() method is not only used for aliases:

Yes but since HTML5, it's allowed.

Well, then I am fine with removing the id-prefix. It is still a BC break though. 😄

@Toflar
Copy link
Member

Toflar commented Feb 17, 2017

I said remove it in the generateAlias() method, NOT the standardize()!

@leofeyer
Copy link
Member

How is that supposed to work? We cannot apply str_replace('id-', '', $alias), because some people might want to start their aliases with id-.

@leofeyer
Copy link
Member

You probably mean adding a third argument to the StringUtil::standardize() method, right?

@Toflar
Copy link
Member

Toflar commented Feb 17, 2017

Yeah, something like that :)

@aschempp
Copy link
Member

aschempp commented Feb 17, 2017 via email

@discordier
Copy link
Contributor

I had the same problem in Catalog back then. Have written some rather big comment back in the day. It definately was related to searching for numeric ids at the same time as searching for textual aliases.

@discordier
Copy link
Contributor

For the record, my comment is over here.
My issue can be reproduced by executing the following queries against a Contao database (provided a page with ID 123 exists):

-- returns dataset with id 123
SELECT p.id, p.alias FROM tl_page p WHERE p.id='123';

-- returns dataset with id 123
SELECT p.id, p.alias FROM tl_page p WHERE p.id=123;

-- returns dataset with id 123
SELECT p.id, p.alias FROM tl_page p WHERE p.id='123-testing';

-- returns no dataset
SELECT p.id, p.alias FROM tl_page p WHERE p.id='id-123-testing';

As you see, all queries performing something like:

SELECT p.id, p.alias FROM tl_page p WHERE p.id='123-testing' OR p.alias='123-testing';

Will randomly either return the dataset with ID 123 OR the one with alias 123-testing.

@Toflar
Copy link
Member

Toflar commented Feb 27, 2017

So that's because the db truncates everything that is not a digit and then it matches. Funny. Is that due to the non-strict mode? Anyway, your Model::findByIdOrAlias() should not have that issue thanks to the is_numeric() check:

// Try to load from the registry
if (is_numeric($varId) && empty($arrOptions))
{
	$objModel = \Model\Registry::getInstance()->fetch(static::$strTable, $varId);

	if ($objModel !== null)
	{
		return $objModel;
	}
}

$t = static::$strTable;

$arrOptions = array_merge
(
	array
	(
		'limit'  => 1,
		'column' => array("($t.id=? OR $t.alias=?)"),
		'value'  => array((is_numeric($varId) ? $varId : 0), $varId),
		'return' => 'Model'
	),

	$arrOptions
);

return static::find($arrOptions);

But can anybody tell me why the code does an OR query which is slower than doing the following anyway??

diff --git a/system/modules/core/library/Contao/Model.php b/system/modules/core/library/Contao/Model.php
index 515018f..d855f15 100644
--- a/system/modules/core/library/Contao/Model.php
+++ b/system/modules/core/library/Contao/Model.php
@@ -807,8 +807,10 @@ abstract class Model
 	 */
 	public static function findByIdOrAlias($varId, array $arrOptions=array())
 	{
+		$isAlias = is_numeric($varId);
+
 		// Try to load from the registry
-		if (is_numeric($varId) && empty($arrOptions))
+		if ($isAlias && empty($arrOptions))
 		{
 			$objModel = \Model\Registry::getInstance()->fetch(static::$strTable, $varId);
 
@@ -825,8 +827,8 @@ abstract class Model
 			array
 			(
 				'limit'  => 1,
-				'column' => array("($t.id=? OR $t.alias=?)"),
-				'value'  => array((is_numeric($varId) ? $varId : 0), $varId),
+				'column' => $isAlias ? "$t.alias=?" : "$t.id=?",
+				'value'  => $varId,
 				'return' => 'Model'
 			),

@aschempp
Copy link
Member

aschempp commented Feb 27, 2017 via email

@Toflar
Copy link
Member

Toflar commented Feb 27, 2017

but it must never be JUST a number.

Exactly. We should keep the id- prefix if it's only a number.

@leofeyer
Copy link
Member

Ok, so I'll adjust the code to allow non-numeric aliases starting with a number.

@Toflar Do you want to create a separate ticket/PR to change the OR query?

@Toflar
Copy link
Member

Toflar commented Mar 20, 2017

See #729

@leofeyer
Copy link
Member

Thank you @Toflar.

@leofeyer
Copy link
Member

Fixed in 5ec030b.

@leofeyer
Copy link
Member

I have changed the implementation in f14eaf2 so the logic applies to the generateAlias() method only.

@aschempp
Copy link
Member

Unfortunately, I don't think that's gonna work. 123foo is casted to a number when compared on database level with the id field and would still return wrong results!

@Toflar
Copy link
Member

Toflar commented Mar 21, 2017

Which is why we don't have the OR query anymore...

@leofeyer
Copy link
Member

But the query change has been assigned to Contao 4.4 (new feature). Do you want me to merge it into version 4.3.6 instead?

@aschempp
Copy link
Member

Which is why we don't have the OR query anymore...

I know, but as said, that only applies to the core things! Extensions might still use the old query mode and rely on standardize

@Toflar
Copy link
Member

Toflar commented Mar 21, 2017

No, this change should be part of 4.4!

@leofeyer
Copy link
Member

Ok, but we can still leave it in the code base, can't we?

@leofeyer
Copy link
Member

leofeyer commented Mar 21, 2017

Hm, the following does not work on my system:

SELECT * FROM tl_page WHERE alias='403-foo';

How do I reproduce the initial problem?

@leofeyer leofeyer modified the milestones: 4.4.0, 4.3.6 Mar 21, 2017
@leofeyer leofeyer reopened this Mar 21, 2017
agoat pushed a commit to agoat/contao-core-bundle that referenced this issue Apr 10, 2017
* Make Symfony 3.2 the minimum requirement (see contao#630).

* Set the encryption key from the kernel secret by default (see contao#660).

* Stop using the deprecated QuestionHelper::setInputStream() method.

* Update Dropzone to version 4.

* Prefer the caret operator over the tilde operator in the composer.json file.

* Add the contao.root_dir parameter (see contao#662).

* Update the change log.

* Stop using the contao-components/all meta package.

* Update the README.md file.

* Deprecate the contao:version command (see contao#668).

* Update the installation path.

* Auto-select the active page in the quick navigation/link module (see contao/core#8587).

* Look up the form class and allow to choose the type (see contao/core#8527).

* Add PHP Toolbox support (see contao#565).

* Remove the arrow brackets in the book navigation template (see contao/core#8607).

* Add a bottom padding to the buttons layer.

* Add the contao.web_dir parameter (see contao/installation-bundle#40).

* Fix the tests.

* Match security firewall based on request scope (see contao#677).

* Fix an issue found by Scrutinizer.

* Use the contao.web_dir parameter in the Combiner (see contao#679).

* Fix the tests.

* Add stripRootDir() method to System class (see contao#683).

* Add the contao.image.target_dir parameter (see contao#684).

* The ContaoCoreExtension::overwriteImageTargetDir() is not deprecated.

* Support custom backend routes (see contao#512).

* Use the scope matcher instead of checking the request attribute (see contao#688).

* Replace every occurrence of $contaoFramework with $framework.

* Fix an issue found by Scrutinizer.

* Fix deprecations in unit tests (see contao#687).

* Added a DBAL field type for UUIDs (see contao#415).

* Support importing form field options from a CSV field (see contao#444).

* Fix the coding style and the unit tests.

* Add the Doctrine field type in the config.yml file.

doctrine:
    dbal:
        types:
            binary_string:
                class: "Contao\\CoreBundle\\Doctrine\\DBAL\\Types\\BinaryStringType"
                commented: true

* Add a basic unit test for the BackendCsvImportController class.

* Update the change log.

* Fix rebuilding the search index (see contao#689).

* Also handle „no origin“ and „empty origin“ in the CORS provider.

* Remove an unused use statement.

* Remove the security.yml file and update the README.md file.

* Improve the e-mail extraction in the text element (thanks to Martin Auswöger).

* Rename the Test namespace to Tests.

* Update the composer.json file.

* Update the .php_cs file.

* Raise the minimum PHP version to 5.6 (see contao#701).

* Support using objects in callback arrays (see contao#699).

* Use try-finally blocks to close all output buffers when downloading a file (see contao#714).

* Fix the coding style.

* Only prefix an all numeric alias when standardizing (see contao#707).

* Adjust the test namespaces.

* Allow to manually pass a value to any widget (see contao#674).

* Add a change log entry and fix the tests.

* Disable the picker buttons if the main window does not show a picker.

* Use the file manager instead of the file picker.

* Use the site structure instead of the page picker.

* Always show the selected nodes.

* Add the menu builder.
@leofeyer
Copy link
Member

See #729.

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

6 participants