[BUGFIX] Harden XPath query to limit search for page tree node #479
Conversation
I propose we move this to a separate change (following the "only one kind of change per PR" rule). |
Sure, we can do that. The reason why I combined both changes into one PR is because the change of the XPath query makes tests in TYPO3 core fail due to the fact those static properties are not accessed correctly. This additional change was necessary to make tests in https://review.typo3.org/c/Packages/TYPO3.CMS/+/80039 green again 😉 But I'm also totally fine with splitting into two separate PRs. |
|
@eliashaeussler Don't act for now !!! In TF we usually do not merge it as merge-commits, so as long as we The comment from @oliverklee in general is valid and a good advice In the worst case we (I) would cherry-pick the commits anyway. Only point on quick view, it would be nice to have one or to sentence |
eliashaeussler
force-pushed
the
fix/page-tree-xpath
branch
from
9f47062
to
7a443e4
Compare
eliashaeussler
force-pushed
the
fix/page-tree-xpath
branch
from
7a443e4
to
2e6b008
Compare
|
@sbuerk Do you need anything else from me to continue here? :) |
Even if page tree nodes are queried from a given context node, the used XPath query searches for all elements in the document because of the `//` syntax. This may lead to possible test failures in case the queried node text appears multiple times in the document. In order to avoid such behaviors and actually limit the XPath query to the given context, it is now prefixed with a dot (`.`). In addition, only following siblings of the current node are included since page tree nodes are presented as flat node structure. This way, the current context is now actually taken into account.
This changes all property accesses within AbstractPageTree to use static instead of self. Since the class is abstract and therefore subject to class inheritance, its properties may also change in subclasses. Using self to access them would hide potentially overridden properties (late static binding).
eliashaeussler
force-pushed
the
fix/page-tree-xpath
branch
from
2e6b008
to
4f4e1b7
Compare
Even if page tree nodes are queried from a given context node, the currently used XPath query searches for all elements in the document instead. That's because of the
//syntax, which searches in the whole document, regardless of which context is given. This may lead to possible test failures in case the queried node text appears multiple times in the document, for example when trying to access the root page tree node whose text appears twice on the page – next to the TYPO3 logo in the topbar and as root node in the page tree. With the current implementation, the XPath query would match the text in the topbar instead of the text of the root node.In order to avoid such behaviors and actually limit the XPath query to the given context, it is now prefixed with a dot (
.). In addition, only following siblings of the current node are included since page tree nodes are presented as flat node structure. This way, the current context is now actually taken into account.As an additional fix, property accesses of static properties in
AbstractPageTreeis now changed fromself::tostatic::to properly respect overridden properties in extended classes. This is especially necessary for the overridden properties in TYPO3 core'sFileTree.Patch in TYPO3 core: https://review.typo3.org/c/Packages/TYPO3.CMS/+/80039
💡 If this PR will be merged, I'll update the TYPO3 core patch accordingly.