Bug #89295
closed
Typo3QuerySettings set as DefaultQuerySettings in a Repositories initializeObject Method are not honored
100%
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
Updated by Frank Berger about 5 years ago
- Tags set to extbase repository
- Complexity set to medium
Updated by Alexander Schnitzler almost 5 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.
Updated by Frank Berger almost 5 years ago
- Assignee set to Alexander Schnitzler
The Problem is here:
This is called before the defaultQuerySettings are applied in
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
Updated by Frank Berger almost 5 years ago
sorry, in hindsight - which is 20-20 - I should have lead with that..
Updated by Georg Ringer over 4 years ago
- Related to Bug #88372: Extbase QuerySettings ignores SiteConfiguration language settings added
Updated by Markus Hofmann about 4 years ago
I have written a patch for fixing this issue in v10.
Updated by Bastian Stargazer almost 4 years ago
I can confirm this issue in v10. Hope the fix will be applied soon.
Updated by Christian Kuhn over 3 years ago
- Related to Bug #83296: Doctrine respects wrong storage PIDs for sys_category added
Updated by Christian Kuhn over 3 years ago
- Assignee deleted (
Alexander Schnitzler)
Updated by Christian Kuhn over 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.
Updated by Gerrit Code Review over 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
Updated by Gerrit Code Review over 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
Updated by Gerrit Code Review over 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
Updated by Christian Kuhn over 3 years ago
- Status changed from Under Review to Resolved
- % Done changed from 0 to 100
Applied in changeset b81a4b853e369ea2234eaa4f1e131163e52605ac.
Updated by Bastian Stargazer over 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.
Updated by Christian Kuhn over 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.
Updated by Bastian Stargazer over 3 years ago
Very true, thanks for clarification!
Updated by