Project

General

Profile

Actions

Bug #89295

closed

Typo3QuerySettings set as DefaultQuerySettings in a Repositories initializeObject Method are not honored

Added by Frank Berger over 4 years ago. Updated over 2 years ago.

Status:
Closed
Priority:
Should have
Assignee:
-
Category:
Extbase
Target version:
-
Start date:
2019-09-27
Due date:
% Done:

100%

Estimated time:
TYPO3 Version:
9
PHP Version:
7.2
Tags:
extbase repository
Complexity:
medium
Is Regression:
Sprint Focus:

Description

The premise is that an Extbase Repository has been extended from Repository, and the initializeObject Method is used to set Typo3QuerySettings and stored in $this->setDefaultQuerySettings, for example like this:

class EventRepository extends Repository
{
    public function initializeObject()
    {
        $querySettings = $this->objectManager->get(Typo3QuerySettings::class);
        $context = GeneralUtility::makeInstance(Context::class);
        $querySettings->setLanguageUid($context->getPropertyFromAspect('language','id'));
        $querySettings->setLanguageOverlayMode(1);
        $this->setDefaultQuerySettings($querySettings);
    }
}

The reason why we set for example the language like this has a different issue, which will be a different bug report. This is just an example.

The expected behaviour now is that any method using $this->createQuery(), including the magic findX methods and findAll will now use these settings.

This does not happen as in Generic\QueryFactory->create a new DefaultQuery is generated.

public function create($className)
{
         $query = $this->objectManager->get(\TYPO3\CMS\Extbase\Persistence\QueryInterface::class, $className);
         $querySettings = $this->objectManager->get(\TYPO3\CMS\Extbase\Persistence\Generic\QuerySettingsInterface::class);

This has several side effects. Parts of 'our' defaultQuerySettings are used, parts of the new one are used. And anything related to the storagePageIds will fail (no storagePageId will be set in the querySettings, not from typoscript or the the pages field or flexform)

We patched in our project the call-chain down to Generic\QueryFactory->create so the repositories defaultQuerySettings will be passed through the instances and if not null will be used instead if a new instance.

The patch is attached.

The System behaves as expected after that, honoring the settings set in a initializeObject method, even for the magic Methods


Files

extbase-querysettings.diff (4.44 KB) extbase-querysettings.diff Patchfile for this issue Frank Berger, 2019-09-27 16:20
extbase-querysettings-10.diff (4.54 KB) extbase-querysettings-10.diff Markus Hofmann, 2020-11-09 18:46

Related issues 2 (0 open2 closed)

Related to TYPO3 Core - Bug #88372: Extbase QuerySettings ignores SiteConfiguration language settingsClosed2019-05-16

Actions
Related to TYPO3 Core - Bug #83296: Doctrine respects wrong storage PIDs for sys_categoryClosed2017-12-12

Actions
Actions #1

Updated by Frank Berger over 4 years ago

  • Tags set to extbase repository
  • Complexity set to medium
Actions #2

Updated by Alexander Schnitzler about 4 years ago

  • Status changed from New to Needs Feedback

I cannot reproduce this issue.
It's true that the query factory does not respect query settings, but in \TYPO3\CMS\Extbase\Persistence\Repository::createQuery, which is called by the findAll (and magic) method(s), the defaultQuerySettings (if present) are copied over to the query object.

Please see https://github.com/TYPO3/TYPO3.CMS/blob/v9.5.13/typo3/sysext/extbase/Classes/Persistence/Repository.php#L201 for more information.

If I misunderstood the problem, please provide further feedback.

Actions #3

Updated by Frank Berger about 4 years ago

  • Assignee set to Alexander Schnitzler

The Problem is here:

https://github.com/TYPO3/TYPO3.CMS/blob/v9.5.13/typo3/sysext/extbase/Classes/Persistence/Generic/QueryFactory.php#L80

This is called before the defaultQuerySettings are applied in

https://github.com/TYPO3/TYPO3.CMS/blob/v9.5.13/typo3/sysext/extbase/Classes/Persistence/Repository.php#L201

Which means if you just use defaultQuerySettings you will override any preconfigured or contextrelated storagePageIds

therefore either the setting of the storagePageIds need to be done later - i.e. in Repository->createQuery after the clone OR the defaultQuerySettings need to be applied before, like in the patch I suggested..

sorry if that was not clear in my initial report

Actions #4

Updated by Frank Berger about 4 years ago

sorry, in hindsight - which is 20-20 - I should have lead with that..

Actions #5

Updated by Georg Ringer almost 4 years ago

  • Related to Bug #88372: Extbase QuerySettings ignores SiteConfiguration language settings added
Actions #6

Updated by Markus Hofmann over 3 years ago

I have written a patch for fixing this issue in v10.

Actions #7

Updated by Bastian Stargazer about 3 years ago

I can confirm this issue in v10. Hope the fix will be applied soon.

Actions #8

Updated by Christian Kuhn almost 3 years ago

  • Related to Bug #83296: Doctrine respects wrong storage PIDs for sys_category added
Actions #9

Updated by Christian Kuhn almost 3 years ago

  • Assignee deleted (Alexander Schnitzler)
Actions #10

Updated by Christian Kuhn almost 3 years ago

  • Status changed from Needs Feedback to New

Hey. I had a look at the query settings magic in extbase and stumbled upon this issue.

I think I understand what happens, what this issue is about, had a look a the proposed patches, and came to a conclusion.

Let me sum up the situation:

Usually, when no defaultQuerySettings are set in a repository, then createQuery() creates some query settings. This is triggered through QueryFactory->create(). It applies settings derived from TCA of this table (via DataMapper) like respectStoragePage (from TCA ctrl rootLevel), and it also sets storagePids from TS configuration.

If you now set a defaultQuerySettings in a repository (eg. via initializedObject()), these query settings are used instead (Repository->create() does this after calculating own query settings, which is kinda stupid, too). They fully override the above calculated settings. This basically means: If you set defaultQuerySettings, you're saying "I know better" and you then need to take care of other things that would be usually applied, too. For instance, you'll have to apply storage pid restrictions yourself (again), if you want so.

The proposed patches try to merge both QuerySettings objects together by handing defaultQuerySettings over to the factory. This however is problematic, too: If your defaultQuerySettings set a storage pid that is NOT identical to that from configuration (because it fetches the value from elsewhere / dynamically), then the factory would happily override your storagePid again. The same is true for respectStoragePage: Lets say you setRespectStoragePage(true) in defaultQuerySettings for whatever reason (maybe you don't want categories from pid 0), then the factory would happily set this to false again, if TCA ctrl rootLevel = 1 is set. So this approach does not work well. The main issue is, that QuerrySettings does not know which setters have been explicitly called, so it can't know which default settings should be applied and which shouldn't. Implementing such a magic doesn't sound right, too: This query settings magic is already quite complex and often does wrong things, so I'm not too fond of adding 'dirty / explicitely triggered' handling to this class.

I came to the conclusion, we shouldn't change the behavior at this point: Instead I propose a patch to document it: "Hey, if you set defaultQuerySettings, then you need to apply other settings yourself since you shut down internal magic this way" ... something like that. I'll push a patch in this direction.

Actions #11

Updated by Gerrit Code Review almost 3 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/c/Packages/TYPO3.CMS/+/69324

Actions #12

Updated by Gerrit Code Review almost 3 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/c/Packages/TYPO3.CMS/+/69324

Actions #13

Updated by Gerrit Code Review almost 3 years ago

Patch set 1 for branch 10.4 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/69274

Actions #14

Updated by Christian Kuhn almost 3 years ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100
Actions #15

Updated by Bastian Stargazer almost 3 years ago

[...] so it can't know which default settings should be applied and which shouldn't.

Wouldn't a flag help like $mergeSettings = true|false to indicate if the given settings are "complete" or should be merged with existing ones? This way could also act as fall-back for the current behavior e.g. if $mergeSettings is true by default.

Actions #16

Updated by Christian Kuhn almost 3 years ago

Thx Bastian. Your prpoposal is basically a 'dirty' handling - something that remembers if the object has been fiddled with, maybe not automatically determined, but manually set.
Of course one can do that.
Question is only if it's worth it: It still requires the changes to the interfaces, similar to what the two patches above do. And it needs a dev to set this flag if he fiddles with one property that would overridden otherwise. So dev needs to know which are overridden by the framework.
Basically, we're talking about storagePid here - it will be the one a developer has to take care of. I'm not sure if that's really worth the effort. Codewise it would be something like that: $settings->setStoragePids('1,42'); $settings->dontChangeMyStuffAgain(true); ... So storage pid would need to be taken care of, anyways. And then there's not much point in handing this over to the factory: It does not simplify dev's work.
I tend to keep the current solution for now.

Actions #17

Updated by Bastian Stargazer almost 3 years ago

Very true, thanks for clarification!

Actions #18

Updated by Benni Mack over 2 years ago

  • Status changed from Resolved to Closed
Actions

Also available in: Atom PDF