Bug #56150: Exception with "Hidden" sys_file_references when using levelmedia - RootlineUtility::enrichWithRelationFields ignores disable field - TYPO3 Core - TYPO3 Forge

Actions

Copy link

Bug #56150

closed

Exception with "Hidden" sys_file_references when using levelmedia - RootlineUtility::enrichWithRelationFields ignores disable field

Added by Christian Reiter about 10 years ago. Updated over 5 years ago.

Status:

Closed

Priority:

Must have

Assignee:

Category:

File Abstraction Layer (FAL)

Target version:

next-patchlevel

Start date:

2014-02-20

Due date:

% Done:

100%

Estimated time:

TYPO3 Version:

6.1

PHP Version:

5.4

Tags:

Complexity:

Is Regression:

Sprint Focus:

Description

Test scenario in TYPO3 6.2:

A Page (e.g. ID=15) has 2 sys_file_reference relations (pages.media)

Frontend output is via levelmedia, for instance

file {
       import.data = levelfield:1, media
       treatIdAsReference = 1
       import.listNum = 0
       }

In 6.2 whichever reference is sorted to the top in the backend is output as expected so far.
(in 6.1 this does not work because sorting_foreign was ignored http://forge.typo3.org/issues/46383#change-204405) but that is fixed for 6.2 with https://review.typo3.org/#/c/26727/

Now the editor uses the "Light bulb" icon to hide the first reference.

Expected result: The second reference should be shown now.

Observed result: Exception thrown "#1317178794: No fileusage (sys_file_reference) found for given UID"

Reason:

levelmedia means the rootline is checked for the references.
They are put there by the function enrichWithRelationFields in \TYPO3\CMS\Core\Utility\RootlineUtility
This loads relations by generating a SQL query.
This query checks that record isn't deleted=1 and in 6.2 also orders by foreign_sortby (in 6.1 it doesn't)
However it does not check for disabled=1

This means that both sys_file_reference entries go into the "enriched" rootline data.

Then the import.listNum = 0 means that the first one is returned.

That one is hidden.

So when we get to TYPO3\CMS\Core\Resource\ResourceFactory::getFileReferenceObject the exception is thrown because only the hidden reference has arrived.

This is pretty confusing for editors as they are using seemingly harmless, legal operations in TYPO3 ("hiding") and this ends up killing entire page trees in the frontend. As when the rootline is checked of course all the pages below can be affected. A typical application of levelmedia is banners for a chapter and so this situation could "exceptionalize" an entire website chapter just due to hinding a banner.
This is hitting live projects with 6.1

In 6.2 they can solve that by resorting the references but in 6.1 they can't

It would be easy to change enrichWithRelationFields to respect $GLOBALS['TCA'][$table]['ctrl']['enablecolumns']['disabled'] in general or for sys_file_reference specifically.

However the comment in TYPO3\CMS\Frontend\Page\PageRepository::getRootLine says:

NOTICE: This function only takes deleted pages into account! So hidden, starttime and endtime restricted pages are included no matter what.

So this seems to be necessary/desired behaviour?

Is it possible to make levelmedia in 6.1/6.2 "hidden-safe" without breaking that behaviour (e.g. by applying the hidden check only for sys_file_reference relations in enrichWithRelationFields )

Or could at least the exception "No fileusage (sys_file_reference) found for given UID" be replaced with a more graceful failure that allows the page to be rendered? Editors will then just scratch their heads why there is no image instead of going into panic.

This was all not a problem in 4.x because pages.media was just a comma separated list of values, there was no "hiding" of a relation from that, and no foreign sorting either. So checking hidden was indeed irrelevant for levelmedia in 4.x

Related issues 1 (0 open — 1 closed)

Actions

Copy link

Updated by Markus Klein about 10 years ago

Status changed from New to Accepted
Priority changed from -- undefined -- to Must have
Target version set to next-patchlevel
TYPO3 Version changed from 6.2 to 6.1

Hi Christian,

thanks for this in-depth report!

Yes, the foreign_sortby was introduced by me, but not backported, as it might be a breaking change.

I fully agree that the exception, which is ok IMO, must be caught and must not break the FE.

The general question, whether getRootLine() should all enable fields has a lot of consequences probably, that's why I've to hand this over to people with more in-depth knowledge about this area in the Core.

Since this is also a problem for 6.1 live sites, as you wrote above, I'm setting the version to 6.1 for this ticket.

Regards Markus

Actions

Copy link

Updated by Christian Reiter about 10 years ago

For live 6.1 sites I've just made a quixk and dirty xclass fix. It just backports the enrichWithRelationFields from current 6.2 (importing the solution for #46383 to 6.1) and adds disableClause.

This delivers correct behaviour for a 6.1 site affected by the bug.

sorting OK
All references hidden - no exception, behaviour as if no refs
one reference hidden, one visible - visible one picked by listNum=0 regardless of sorting

Question -

is RootlineUtility only used in the Frontend or also called from the Backend?

If it's also used from the BE I'd check the TYPO3_MODE as "hidden" should not be "invisible" in the backend.

Actions

Copy link

Updated by Markus Klein about 10 years ago

I fear it is also used in BE. But I'm not sure.

Actions

Copy link

Updated by Frans Saris about 10 years ago

Assignee deleted (~~Frans Saris~~)

Actions

Copy link

Updated by Christian Reiter about 10 years ago

Hi Markus,

In TYPO3\CMS\Core\Utility\RootlineUtility::__construct there is a check for the $GLOBALS['TSFE']->sys_page object for the page context, maybe by checking for this in enrichWithRelationFields we could avoid side effects when the RootlineUtility is used for something else than rendering a frontend page?

Actions

Copy link

Updated by Markus Klein about 10 years ago

If TS is parsed in BE (like for extbase) I guess the sys_page is present as well.

Actions

Copy link

Updated by Christian Reiter about 10 years ago

So then I guess the information that is available within the scope of that function is possibly not sufficient to be sure that everything is safe.

I see two possibilities right now,

add some kind of parameter that says "I explicitly want to enrich a rootline for FE rendering" - although I don't really like the concept of adding such parameters
- perhaps from TYPO3\CMS\Frontend\ContentObject\ContentObjectRenderer::rootLineValue which is called to make the rootline for levelmedia
- reason: Comment for this function says "@todo Define visibility"
  - so someone seems to be aware that this function returns a rootline with undefined treatment of visibility?
- This function "_rootLineValue_" is used exclusively from TYPO3\CMS\Frontend\ContentObject\ContentObjectRenderer::getData
  - That one again has "@todo Define visibility" in the comment
  - getData is basically just a big switch construct which treats the different TS functions like leveltitle,levelmedia,leveluid,levelfield etc...
    so it is pretty much limiting things to a Typoscript Frontend context where we would want the change to be effective
- Also since this is basically the cObj class, it should be frontend, or at least simulation of frontend?

Just change it and test a lot... ;)

I've had no problem with the Xclass solution that adds the visibility/disablefield check so far on a live project but of course that is ony one site.

Actions

Copy link

Updated by Markus Klein about 10 years ago

@todo Define visibility

refers to the protection of the method itself. This was added when all the function got access modified. Back then it was not known which function are protected or not, so all of them got public.
It will take years to finally make those protected, which are actually used only internally and we never know, if such a function has been used by an extension.
You'll find these todos everywhere in Core.

I propose to simply add it for now to have a solution in Gerrit. Then it can be tested and can be part of the next beta version, before backporting it to 6.1.

Can you push a patch?

Actions

Copy link

Updated by Christian Reiter about 10 years ago

Ok thanks for the explanation on "visibility" :)

I found the "Contribution Walkthrough Tutorial" on wiki.typo3.org, I'll digest that and proceed.

Actions

Copy link

#10

Updated by Gerrit Code Review about 10 years ago

Status changed from Accepted 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/28277

Actions

Copy link

#11

Updated by Gerrit Code Review about 10 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/28277

Actions

Copy link

#12

Updated by Gerrit Code Review about 10 years ago

Patch set 3 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/28277

Actions

Copy link

#13

Updated by Gerrit Code Review about 10 years ago

Patch set 1 for branch TYPO3_6-1 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/28400

Actions

Copy link

#14