Bug #83581
open
Logical error while checking validity of a shortcut
100%
Description
The function getSubpagesForPages in Class TYPO3\CMS\Frontend\Page\PageRepository uses the constraints given to filter the resulting subpages to also check if the target of a shortcut is valid. The validity of a shortcut target (as the comment line above the function call in question says: "If shortcut, look up if the target exists and is currently visible") should only depend on its existence and visibility and not any other constraints used to filter the result of getSubpagesOfPages (e.g. a shortcut itself).
Updated by Gerrit Code Review over 6 years ago
- Status changed from New to Under Review
Patch set 1 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/55377
Updated by Gerrit Code Review over 6 years ago
Patch set 1 for branch TYPO3_8-7 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/55378
Updated by Stephan Großberndt over 6 years ago
To test this create a shortcut to a page (visible in menu), activate the 'hidden in menu'-option for the page itself and use it in a generated menu.
- before: the shortcut would not be displayed as part of the menu
- after: the shortcut will now be displayed corrertly as part of the menu
Updated by Gerrit Code Review over 6 years ago
Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/55377
Updated by Mathias Schreiber about 6 years ago
- Status changed from Under Review to Needs Feedback
Can you please describe the usecase in more detail?$additionalWhere only applies if the respective TMENU gets an additionalWhere.
The entire point of additionalWhere is to apply... well... an addition to the whereclause of all items of a menu.
If we removed constraint from TMENU we'd get the same bug report the other way around :)
Benni and I spent quite some time on trying to understand the usecase, but everything, we came up with, worked.
This is why we think your usecase is something we didn't think of.
Please share it, ideally with a couple of screenshots and the respective Typoscript Menu config.
Updated by David Otto about 6 years ago
- Assignee changed from David Otto to Mathias Schreiber
In this case the additionalWherClause is applied to the checking of shortcut targets. This means all constraints are used on the targets. I understand this check just as the comment above it says "If shortcut, look up if the target exists and is currently visible". So checking for anything else is false logic in my opinion.
The exact use-case for me was a menu generated via VHS that stopped showing shortcuts to hidden in menu pages within the menu after upgrading from TYPO3 6.2 to 8.7. After a while i noticed, that checking if shourtcuts are existent and not deleted is absolutely fine, but using the menu constraints on the shortcut targets produces this behaviour.
Updated by Riccardo De Contardi almost 6 years ago
- Status changed from Needs Feedback to New
Updated by Benni Mack about 5 years ago
- Target version changed from next-patchlevel to Candidate for patchlevel
Updated by Mathias Schreiber about 2 years ago
- Sprint Focus set to Remote Sprint
Updated by Gerrit Code Review almost 2 years ago
- Status changed from New to Under Review
Patch set 1 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/74357
Updated by Gerrit Code Review almost 2 years ago
Patch set 2 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/74357
Updated by Gerrit Code Review almost 2 years ago
Patch set 3 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/74357
Updated by Larry Garfield almost 2 years ago
I discussed this issue with Mathias a bit, and went code diving. We think there is a valid argument to having the shortcut not fully inherit the existential state of its target, since mount points already provided effectively that. So it's worth considering.
However, the issue is not (at least in the current code, a few years later) with $additionalWhere; that variable is an SQL fragment, so trying to parse it to detect what's inside and change it is a fools errand and begging for bizarre bugs. However, that's not actually how shortcuts are filtered right now. Instead, there's a separate call to checkValidShortcutOfPage() that does the filtering, via very inefficient SQL. That's controlled by a boolean argument to getSubpagesForPages().
That means disabling that behavior generally is a trivial matter of adding a false to where that method is called from, and I've filed a patch to do so. (99% of the patch is updating the affected test.)
Whether that is wise or not, I am still not convinced; as the test demonstrates, the implications are non-small. I defer to the Core Mergers on whether that patch should be polished and merged or this issue closed as by-design. Either approach, I think, has a valid argument to be made.
Updated by Gerrit Code Review almost 2 years ago
Patch set 4 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/74357
Updated by Oliver Hader over 1 year ago
- Status changed from Under Review to New