-
Notifications
You must be signed in to change notification settings - Fork 17
IBX-10936: Implemented fetching field definitions from an expression #1765
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 4.6
Are you sure you want to change the base?
Conversation
src/bundle/Controller/ContentTypeFieldsByExpressionController.php
Outdated
Show resolved
Hide resolved
tests/integration/REST/PostLoadFieldDefinitionsFromExpression.php
Outdated
Show resolved
Hide resolved
src/lib/REST/Output/ValueObjectVisitor/ContentType/FieldDefinitionInfoList.php
Outdated
Show resolved
Hide resolved
konradoboza
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from other remarks.
| // Building ContentTypeDomainMapper manually to avoid circular dependency. | ||
| $this->contentTypeDomainMapper = new ContentTypeDomainMapper( | ||
| $contentTypeHandler, | ||
| $contentLanguageHandler, | ||
| $fieldTypeRegistry, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A change was required here due to circular dependency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you are saying that ContentTypeFieldsByExpressionService requires ContentTypeDomainMapper, which also uses ContentTypeFieldsByExpressionService?
This won't do. If it's a service it should NOT be built manually - it creates tech debt, because now you need to remember to update this every time.
What is the exact chain of dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Steveb-p no, ContentTypeDomainMapper doesn't require ContentTypeFieldsByExpressionService.
The issue is that ContentTypeFieldsByExpressionService requires both:
private ContentTypeHandler $contentTypeHandler;
private ContentTypeDomainMapper $contentTypeDomainMapper;
where ContentTypeDomainMapper requires ContentTypeHandler hence circular dependency issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Container should be able to handle this. What's the actual error message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Steveb-p please see https://github.com/ibexa/admin-ui/actions/runs/19164178667/job/54780835840.
It was happening only on OSS. Locally I got OOM when building container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not related to those two services, actually. You can see it in the CI logs:
!! PHP Fatal error: Uncaught Error: Maximum call stack size of 16728064 bytes (zend.max_allowed_stack_size - zend.reserved_stack_size) reached. Infinite recursion? in /var/www/var/cache/behat/ContainerOxSk76G/App_KernelBehatDebugContainer.php:7048
!! Stack trace:
!! #0 /var/www/var/cache/behat/ContainerOxSk76G/App_KernelBehatDebugContainer.php(7104): ContainerOxSk76G\App_KernelBehatDebugContainer->getProxyDomainMapperService()
!! #1 /var/www/var/cache/behat/ContainerOxSk76G/App_KernelBehatDebugContainer.php(7048): ContainerOxSk76G\App_KernelBehatDebugContainer->getRepositoryService()
!! #2 /var/www/var/cache/behat/ContainerOxSk76G/App_KernelBehatDebugContainer.php(7104): ContainerOxSk76G\App_KernelBehatDebugContainer->getProxyDomainMapperService()
!! #3 /var/www/var/cache/behat/ContainerOxSk76G/App_KernelBehatDebugContainer.php(7048): ContainerOxSk76G\App_KernelBehatDebugContainer->getRepositoryService()
!! #4 /var/www/var/cache/behat/ContainerOxSk76G/App_KernelBehatDebugContainer.php(7104): ContainerOxSk76G\App_KernelBehatDebugContainer->getProxyDomainMapperService()
!! #5 /var/www/var/cache/behat/ContainerOxSk76G/App_KernelBehatDebugContainer.php(7048): ContainerOxSk76G\App_KernelBehatDebugContainer->getRepositoryService()
Circular reference is between Repository and ProxyDomainMapperService. So the actual issue lies in service which you are omiting.
I can see that in our definitions files, proxy domain mapper is declared as follows:
Ibexa\Core\Repository\ProxyFactory\ProxyDomainMapperFactory:
arguments:
$proxyGenerator: '@Ibexa\Core\Repository\ProxyFactory\ProxyGeneratorInterface'
Ibexa\Core\Repository\ProxyFactory\ProxyDomainMapper:
factory: ['@Ibexa\Core\Repository\ProxyFactory\ProxyDomainMapperFactory', 'create']
arguments:
$repository: '@Ibexa\Core\Repository\Repository'Try marking ProxyDomainMapperFactory as lazy: true, it should go away - unless OSS for some reason is unable to generate lazy proxies. There is definitely an issue, but it lies not with the services you linked, but rather Repository itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Steveb-p with lazy I'm getting:
In InvalidProxiedClassException.php line 28:
Provided class "Ibexa\Core\Repository\ProxyFactory\ProxyDomainMapperFactory" is final and cannot be proxied
After removing final it works but I'm not sure it's what we want to go with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Interface Proxyfying instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, it works, I'll create a core PR in this case, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/bundle/Controller/ContentTypeFieldsByExpressionController.php
Outdated
Show resolved
Hide resolved
8ee52df to
0e365cc
Compare
|



Related PRs:
ibexa/core#676
https://github.com/ibexa/taxonomy/pull/382
Description:
Summary
ContentTypeFieldsByExpressionServiceInterfaceEndpoint
/api/ibexa/v2/content-type/load-field-definitions-from-expressionapplication/vnd.ibexa.api.FieldDefinitionExpression+json(or+xml)application/vnd.ibexa.api.FieldDefinitionInfoList+json(or+xml)Request Payload
{ "FieldDefinitionExpression": { "expression": "{Content}/{folder}/{name}", "configuration": "text_fields" // Optional } }Example JSON Response
{ "FieldDefinitions": { "_media-type": "application/vnd.ibexa.api.FieldDefinitionInfoList+json", "FieldDefinitionInfo": [ { "_media-type": "application/vnd.ibexa.api.FieldDefinitionInfo+json", "id": "__FIXED_ID__", "identifier": "name", "position": 1, "names": { "value": [ { "_languageCode": "eng-US", "#text": "Name" } ] } } ] } }(The XML variant mirrors the same structure, using
media-typeattributes and<value languageCode="...">elements.)For QA:
Documentation:
Internal, NDR