TYPO3 Core

For ID's, you could require (before matching against ID's)

$id == (int) $id

And for the alias names, an exact match could be required.

If both cases don't match, a 404-error still should be returned.

Also keep in mind that duplicate content needs to be avoided.

Please take a close look how complicated id resolution in core is.
There is/was(?) also a hook in the processing queue.
If you find a solution, please push a patch to our review system.

I did some research:

There's also a way to override the &type param in TYPO3\CMS\Frontend\Controller\TypoScriptFrontendController

// Splitting $this->id by a period (.).
                // First part is 'id' and second part (if exists) will overrule the &type param
                $idParts = explode('.', $this->id, 2);

?id=5.123&type=3  //with 123 for the type instead of 3

There's 4 reasons for a 'page not found' in TypoScriptFrontendController:

$pNotFoundMsg = array(
    1 => 'ID was not an accessible page',
    2 => 'Subsection was found and not accessible',
    3 => 'ID was outside the domain',
    4 => 'The requested page alias does not exist'
);

$this->id is filled via \TYPO3\CMS\Core\Utility\GeneralUtility::_GP('id') (in index_ts.php). This function is just stripping slashes and some cleanup for GET/POST variables. Nothing special.

Plus, there's a function checkAndSetAlias() in TYPO3\CMS\Frontend\Controller\TypoScriptFrontendController

public function checkAndSetAlias() {
    if ($this->id && !\TYPO3\CMS\Core\Utility\MathUtility::canBeInterpretedAsInteger($this->id)) {
        $aid = $this->sys_page->getPageIdFromAlias($this->id);
        if ($aid) {
            $this->id = $aid;
        } else {
            $this->pageNotFound = 4;
        }
    }
}

static public function canBeInterpretedAsInteger($var) {
    if ($var === '' || is_object($var) || is_array($var)) {
        return FALSE;
    }
    return (string) (int)$var === (string) $var;
}

Then, if it's not an Alias (or not), the integer is extracted.

// If $this->id is a string, it's an alias
$this->checkAndSetAlias();
// The id and type is set to the integer-value - just to be sure...
$this->id = (int)$this->id;

For some reason, the

canBeInterpretedAsInteger($this->id)

If this function doing nothing if the id can be "interpreted as integer", then there is no need to execute it here.

More to the point would be sth. like

$this->extractIDParts(); //override type, extract id
if ($this->isInt($this->id) && $this->pageExists((int) $this->id)) {
    //page found by id
    $this->id = (int) $this->id;
    //do something
} elseif ($this->checkAlias()) {
    //page found by alias
    $this->id = ...;
    //do something
} else {
    //no ID and no alias found
    //return 404
}

Then, there's no canBeInterpretedAsInteger() necessary in checkAlias().

As far as I can see, the history of TYPO3 CMS must be considered to not break things. I don't really want to touch that.

That's exactly the point... history, backwards compat :-/

if ($this->id && !\TYPO3\CMS\Core\Utility\MathUtility::canBeInterpretedAsInteger($this->id)) {

Do you mean this line? The check is necessary to distinguish plain numbers (uids) from alias names.

Yes, the way it's implemented makes canBeInterpretedAsInteger(...) necessary here. However, in that place, it looks like a quick-hack to me.

I just don't like the way it's implemented.

And I don't like the implementation of canBeInterpretedAsInteger() for that purpose.

In particular, I don't like that "66%20or%20(1,2)=(select*from(select%20name_const(CHAR,1),name_const(CHAR,1))a)%20--%20and%201=1" is supposed to be an integer. It's not... or only by convention (which I don't like).

Nobody uses that for an ID (hopefully). So it should not be handled as an ID. And if it's not a known alias there is no way you can go. Then it should be 404.

Btw, is

?id=11.123   //id=11, type=123

Instead,

?id=11&type=123   //id=11, type=123

Well the implementation of canBeInterpretedAsInteger() is totally ok. It returns FALSE for 66asdf.

If you find a better implementation, please push a patch to Gerrit, so we can take a look.

It's ok in the sense that it meets the convention. Indeed, there is no bug.

As long as you are happy with that convention, I can hardly find any motivation for a patch.

What "convention" do you refer to?

What's the state of this issue?

@Markus:

I'm referring to the convention that the syntax

    ?id=<ID>.<type>

can be used instead of the explicit version

    ?id=<ID>&type=<type>

====

    canBeInterpretedAsInteger()

looks indeed right by itself.

I reviewed the code again and what looks strange for me is the following in TypoScriptFrontendController

public function fetch_the_id() {
    [...]
    $idParts = explode('.', $this->id, 2);
    $this->id = $idParts[0];

Which means that the ID (as the Integer!) was extracted from the <ID>.<type> string.

Now,

$this->id

contains a valid Integer which can indeed be interpreted as Integer, so canBeInterpretedAsInteger() should return true for $this->id respectively

Therefore,

$this->checkAndSetAlias();

will do nothing, because $this->id is now an Integer and not a string. So CheckAndSetAlias() will not check nor return corresponding page id of the alias and it will not change $this->id even if there was a proper alias for the original id.type syntax. Plus, will it not 404 if the alias is not found (because, as said, it basically does nothing).

Because checkAndSetAlias() does nothing when using the ID.type syntax,

$this->id

is not changed and still the page ID integer which was extracted as the first segment of ID.type.

As far as I can see

- ?id=4.56abc behaves differently than using ?id=4&type=56abc
- ?id=4.56abc will ALWAYS set $this->id=4
- Therefore ?id=4.56abc syntax is not suitable for working with aliases at all (at least not for ID manipulation)
- $this->type is still filled with 56abc for ?id=4.56abc, but skipping the alias checking and handling

I want you to investigate if that is

- wanted behaviour
- documented
- widely in use
- clean code (the latter: it's not!)

or if it's just a bug.

I suggest to

- handle ?id=4.56abc and ?id=4&type=56abc exactly the same (if it's a bug).
- therefore 404 if the alias is not found (which I would have expected for /index.php?id=66%20or%20(1,2)=(select...))
- optionally have a better concept for collisions like ?id=4.56abc&type=collision
- end the mismatch meaning and collision aspect of ID, alias and type, plus allowing multiple syntaxes.
- deprecate the ?id=4.56abc syntax and remove it in the long term

Please reassess this ticket. Now it should be clearer.

contains a valid Integer

I can't follow you.
If your &id=66%20..., then $this->id will not contain an integer.

Indeed, there is no dot in the string. ok, this is another issue.

Then,

->checkAndSetAlias();

should check for an alias. Because the alias does not exist, it should

$this->pageNotFound = 4;

Instead, the page was displayed, because no exception was thrown before

$this->id = (int)$this->id;

and afterwards there was no exception thrown either.

$this->TYPO3_CONF_VARS['FE']['pageNotFound_handling']

would be a candidate because of

if ($this->pageNotFound && $this->TYPO3_CONF_VARS['FE']['pageNotFound_handling']) {
    $pNotFoundMsg = array(
        1 => 'ID was not an accessible page',
        2 => 'Subsection was found and not accessible',
        3 => 'ID was outside the domain',
        4 => 'The requested page alias does not exist'
    );
    $this->pageNotFoundAndExit($pNotFoundMsg[$this->pageNotFound]);
}

I looked it up in the install tool and $this->TYPO3_CONF_VARS['FE']['pageNotFound_handling'] is empty (default).

The install tool hint says that Typo3 interpretes this as

empty (default)
    The next visible page upwards in the page tree is shown.

ok, this is another way of getting duplicate content...

In the end it's

- a surprising default setting, combined with misconfiguration (concerning the initial case)
- a really ugly Typo3 class, containing mismatch in variable usage where I look at. I wonder how you can mantain that. Respect!

You still could investigate if the variable mismatch should be avoided and if it really behaves as wanted and documented. I presented several aspects.

The default setting of pageNotFound_handling could be changed to throw an exception by default. Why?

- a developer is not aware of an error if it's hidden and magically healed.
- it produces duplicate content.
- eventually it produces wrong results when displaying the "next" page. What is "next"?
- the behaviour of Typo3 is really unique here. I was so surprised that I thought that it must be unexpected behaviour.
- it makes me VERY nervous if sql injection code is in the apache logs with status 200.

However: If you love everything the way it is, you can close this ticket.

a really ugly Typo3 class, containing mismatch in variable usage where I look at. I wonder how you can mantain that. Respect!

I fully agree, no doubt, but did you ever have a look at the log? Lots of code stems from Kasper's time dating 10+ years back.

We are really looking for more man power to get that code nice and readable. If you can help out, that would be really nice.
Keep in mind though that his is really critical code and changing a single line might already break something for somebody.
We always have to keep the balance between "rewriting it" and "not overlooking a nasty edge-case".
Improving something gives little community feedback, breaking a tiny edge-case is a rant guarantee. That's OS life ;-)

The default setting of pageNotFound_handling could be changed to throw an exception by default. Why?

I agree!

Please get your hands dirty and improve. This not our (Core dev's) code, but Our (community) code, so everybody is welcome to improve.
http://wiki.typo3.org/CWT is a starting point to get your environment ready.

I came in touch with Typo3 in 2003, so I know it's old.

Now that Typo3 is continued to be maintained separately (without migration path to Typo3 NEOS and a sudden death of Typo3 CMS) I would be WAYS more offensive in respect to

- coding style (everybody uses PSR-X standard and I also pretty much like symfony coding style)
- refactoring in respect to maintainability (readable code, testability, dependency injection, ...)
- the arrays typo3 uses everywhere
- underlying modern php framework
- consolidation: remove as much as possible!
- ...

As said, I will not edit one of the most important classes without even knowing how it SHOULD behave. For instance, I can just look at what it actually does. That's not easy either...

I would also see a commitment of the community to bring Typo3 up-to-date technically. That does not mean it will be done instantly, maybe started after 7 LTS. Then, there should be a discussion/decision what should be done and a priority list/roadmap which aspect is touched next.

Some things would be easy to do like using PHP constants where possible. That's nothing new in PHP, doesn't break anything and can be done incrementally. Extension developers would love it, e.g. have a look at https://github.com/mblaschke/TYPO3-metaseo/issues/70 How should this mismatch be maintained for another 10+ years?

Don't know, but for me this sounds like "you missed all development" since 6.0 ;-) (no offense please)

commitment of the community to bring Typo3 up-to-date technically

This is basically what we're trying to do since 6.0. But we're talking about > 3 million LOC, which must be kept backwards compatible as much as possible.

• we have a very high level on the code quality we expect for new code
• we strive to add tests whenever reasonably possible
• basically most patches are just refactorings, even bugfixes escalate to be refactorings in the end
• underlaying framework => not reasonably possible without rewriting things from scratch => Neos
• we remove a lot => http://docs.typo3.org/typo3cms/extensions/core/latest/

CONSTANTS are real pest when it comes to deprecation. There is no sane way to deprecate old constants. No way to detect usage of a constant and tell the dev that it will be gone in the next version. (same for public class members btw, which we really suffer from => whenever removing a public member you have to declare that as Breaking Change!)

I really suggest you join us on Slack and read a bit in the channels what is going on in Core development. Of course fruitful discussions and suggestions are always welcome.

Doctype stuff: If it bothers you, please get your hands dirty and come up with a blueprint or even a patch on how to improve (even if it is a breaking change). There is still time left till the CMS 7 LTS release, hence time enough to get in things.

That's possible. I went from LTS to LTS in the last time, so I did not get in touch with 6.0/6.1 respectively ;)

I mean, would you accept a patch that breaks the ?id=ID.TYPE syntax? I have no idea if that's widely used. Could someone please scan the entire internet?

Anyways, some incremental refactoring could I do at some (other) place. I need to get in touch with the conventions of Typo3 core first, this is so much different from other PHP projects (which like constants and PSR-x and ...).

PSR-3 is fully implemented.
PSR-4 is fully adopted.

CGL is indeed different (and I personally consider the PSR CGL the biggest mistake PHP has made since a long time), but should be easy to grasp by looking at the existing code (like in any other project as well).

I guess we can consider breaking the <id>.<type> syntax as breaking change.
But we can get a good response by just asking this question on Slack.

That's good news. I already registerd on Slack.

I personal find that the CGL PSR was the best step forward for contribute to different projects. ;-)

Anyway, I would like to close this issue, as the comments aren't left the problem behind this issue.

@Thomas Mayer would you please open a new issue report with the latest findings and if you like to work on this issue assign it to yourself? You can also add me (and maybe Markus) as watcher. And we will help you to get your patch(es) merged. Ok?

Yes, I will open a new ticket.

This ticket got ways too confusing and can be closed then.