Bug #83581: Logical error while checking validity of a shortcut - TYPO3 Core - TYPO3 Forge

Custom queries

Accessibility
Easy tasks
Forge Triage
JavaScript issues
JavaScript Issues (w/o Epics/Stories)
My open issues
No feedback within 90 days
Open Bugs
Open issues of 10
Recently modified
Regressions
Stabilization Issues
Usability or UX

Actions

Copy link

Bug #83581

open

Logical error while checking validity of a shortcut

Added by David Otto almost 7 years ago. Updated almost 2 years ago.

Status:

New

Priority:

Must have

Assignee:

Mathias Schreiber

Category:

Frontend

Target version:

Candidate for patchlevel

Start date:

2018-01-16

Due date:

% Done:

100%

Estimated time:

TYPO3 Version:

PHP Version:

7.0

Tags:

Complexity:

no-brainer

Is Regression:

Sprint Focus:

Remote Sprint

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).

History
Notes
Property changes

Actions

Copy link

Updated by Gerrit Code Review almost 7 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

Actions

Copy link

Updated by Gerrit Code Review almost 7 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

Actions

Copy link

Updated by Stephan Großberndt almost 7 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

Actions

Copy link

Updated by Gerrit Code Review almost 7 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

Actions

Copy link

Updated by Mathias Schreiber almost 7 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.

Actions

Copy link

Updated by David Otto almost 7 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.

Actions

Copy link

Updated by Riccardo De Contardi over 6 years ago

Status changed from Needs Feedback to New

Actions

Copy link

Updated by Benni Mack over 5 years ago

Target version changed from next-patchlevel to Candidate for patchlevel

Actions

Copy link

Updated by Mathias Schreiber over 2 years ago

Sprint Focus set to Remote Sprint

Actions

Copy link

#10

Updated by Gerrit Code Review over 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

Actions

Copy link

#11

Updated by Gerrit Code Review over 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

Actions

Copy link

#12

Updated by Gerrit Code Review over 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

Actions

Copy link

#13

Updated by Larry Garfield over 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.

Actions

Copy link

#14

Updated by Gerrit Code Review over 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

Actions

Copy link

#15

Updated by Oliver Hader almost 2 years ago

Status changed from Under Review to New

Actions

Copy link

Also available in: Atom PDF

Project

General

Profile

TYPO3 Core

Custom queries

Bug #83581

Logical error while checking validity of a shortcut

Updated by Gerrit Code Review almost 7 years ago

Updated by Gerrit Code Review almost 7 years ago

Updated by Stephan Großberndt almost 7 years ago

Updated by Gerrit Code Review almost 7 years ago

Updated by Mathias Schreiber almost 7 years ago

Updated by David Otto almost 7 years ago

Updated by Riccardo De Contardi over 6 years ago

Updated by Benni Mack over 5 years ago

Updated by Mathias Schreiber over 2 years ago

Updated by Gerrit Code Review over 2 years ago

Updated by Gerrit Code Review over 2 years ago

Updated by Gerrit Code Review over 2 years ago

Updated by Larry Garfield over 2 years ago

Updated by Gerrit Code Review over 2 years ago

Updated by Oliver Hader almost 2 years ago