Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Serializer] ObjectNormalizer: throw if PropertyAccess isn't installed#24563
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
xabbuh commentedOct 16, 2017
I think we should also not register the |
dunglas commentedOct 16, 2017
@xabbuh if we do that, will not it be very hard to debug? |
| publicfunction__construct(ClassMetadataFactoryInterface$classMetadataFactory =null,NameConverterInterface$nameConverter =null,PropertyAccessorInterface$propertyAccessor =null) | ||
| { | ||
| if (!class_exists('Symfony\Component\PropertyAccess\PropertyAccess')) { | ||
| thrownewRuntimeException('Unable to use ObjectNormalizer as the Symfony PropertyAcess component is not installed.'); |
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.
PropertyAccess
BoShurik commentedOct 16, 2017
@dunglas otherwise impossible to use |
| publicfunction__construct(ClassMetadataFactoryInterface$classMetadataFactory =null,NameConverterInterface$nameConverter =null,PropertyAccessorInterface$propertyAccessor =null) | ||
| { | ||
| if (!class_exists('Symfony\Component\PropertyAccess\PropertyAccess')) { | ||
| thrownewRuntimeException('Unable to use ObjectNormalizer as the Symfony PropertyAccess component is not installed.'); |
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.
What do you guys think about:
The ObjectNormalizer class requires the "PropertyAccess" component. Install symfony/property-access to use it.
The "Install symfony/property-access" seems important now that even FW users are responsible in general for installing packages that they need.
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.
Good idea, however it will be many other texts to change (I've adapted an existing one, coming from FrameworkBundle IIRC).
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.
Done, I've just added extra quotes aroundsymfony/property-access.
dunglas commentedOct 16, 2017
@xabbuh, after double checking |
fabpot commentedOct 16, 2017
Thank you@dunglas. |
…'t installed (dunglas)This PR was squashed before being merged into the 2.7 branch (closes#24563).Discussion----------[Serializer] ObjectNormalizer: throw if PropertyAccess isn't installed| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | yes| New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? | no| Deprecations? | no <!-- don't forget to update UPGRADE-*.md files -->| Tests pass? | yes| Fixed tickets |#24553| License | MIT| Doc PR | n/aCommits-------b5246a7 [Serializer] ObjectNormalizer: throw if PropertyAccess isn't installed
…t installed (dunglas)This PR was squashed before being merged into the 3.3 branch (closes#24565).Discussion----------[Serializer] YamlEncoder: throw if the Yaml component isn't installed| Q | A| ------------- | ---| Branch? | 3.3 <!-- see comment below -->| Bug fix? | yes| New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? | no| Deprecations? | no <!-- don't forget to update UPGRADE-*.md files -->| Tests pass? | no| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | n/aSimilar to#24563 but for the Yaml encoder (not the same branch).Commits-------7d21caf [Serializer] YamlEncoder: throw if the Yaml component isn't installed
BoShurik commentedOct 18, 2017
@dunglas this exception does not rise when using flex. Is it ok? I've got an exception when trying to access serializer component |
ogizanagi commentedOct 18, 2017
See#24605 |
…omponent isn't installed (ogizanagi)This PR was squashed before being merged into the 2.7 branch (closes#24605).Discussion----------[FrameworkBundle] Do not load property_access.xml if the component isn't installed| Q | A| ------------- | ---| Branch? | 2.7 <!-- see comment below -->| Bug fix? | yes| New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? | no| Deprecations? | no <!-- don't forget to update UPGRADE-*.md files -->| Tests pass? | yes| Fixed tickets |#24563 (comment) <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | N/AThis PR actually aims tofix#24563 (comment) as the exception introduced in the PR can't be reached anyway when using the FrameworkBundle without the property access component as you'll get:> Uncaught Symfony\Component\Debug\Exception\ClassNotFoundException: Attempted to load class "PropertyAccessor" from namespace "Symfony\Component\PropertyAccess".With this fix, you properly get:> The ObjectNormalizer class requires the "PropertyAccess" component. Install "symfony/property-access" to use it.Not sure this change really belongs to a patch release, but the original PR was accepted in the 2.7 branch.Also, I'd rather remove the ObjectNormalizer definition if the component isn't available, as suggested by@xabbuh (#24563 (comment)). But in 2.7, this is the only normalizer registered by default and the [`SerializerPass` throws an exception if no normalizer is registered.](https://github.com/symfony/symfony/blob/2.7/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/SerializerPass.php#L46)To sum up, either:1. we completly prevent using the FrameworkBundle and the serializer without the property access component, even if you don't really care about the ObjectNormalizer because you only use your owns specific ones. (and you'll get the exception hinting to install the property access component)2. we allow using the FrameworkBundle and the serializer without the property access component, so we remove the ObjectNormalizer definition, but the user'll get a `You must tag at least one service as "serializer.normalizer" to use the Serializer service` exception until he configures a normalizer (and we don't get the hint about installing the property access component to enable the ObjectNormalizer. We already have a suggest entry in the composer.json file, though).To me option 2 looks better. WDYT?Commits-------d297e27 [FrameworkBundle] Do not load property_access.xml if the component isn't installed
…omponent isn't installed (ogizanagi)This PR was squashed before being merged into the 2.7 branch (closes #24605).Discussion----------[FrameworkBundle] Do not load property_access.xml if the component isn't installed| Q | A| ------------- | ---| Branch? | 2.7 <!-- see comment below -->| Bug fix? | yes| New feature? | no <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? | no| Deprecations? | no <!-- don't forget to update UPGRADE-*.md files -->| Tests pass? | yes| Fixed tickets |symfony/symfony#24563 (comment) <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | N/AThis PR actually aims tofixsymfony/symfony#24563 (comment) as the exception introduced in the PR can't be reached anyway when using the FrameworkBundle without the property access component as you'll get:> Uncaught Symfony\Component\Debug\Exception\ClassNotFoundException: Attempted to load class "PropertyAccessor" from namespace "Symfony\Component\PropertyAccess".With this fix, you properly get:> The ObjectNormalizer class requires the "PropertyAccess" component. Install "symfony/property-access" to use it.Not sure this change really belongs to a patch release, but the original PR was accepted in the 2.7 branch.Also, I'd rather remove the ObjectNormalizer definition if the component isn't available, as suggested by@xabbuh (symfony/symfony#24563 (comment)). But in 2.7, this is the only normalizer registered by default and the [`SerializerPass` throws an exception if no normalizer is registered.](https://github.com/symfony/symfony/blob/2.7/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/SerializerPass.php#L46)To sum up, either:1. we completly prevent using the FrameworkBundle and the serializer without the property access component, even if you don't really care about the ObjectNormalizer because you only use your owns specific ones. (and you'll get the exception hinting to install the property access component)2. we allow using the FrameworkBundle and the serializer without the property access component, so we remove the ObjectNormalizer definition, but the user'll get a `You must tag at least one service as "serializer.normalizer" to use the Serializer service` exception until he configures a normalizer (and we don't get the hint about installing the property access component to enable the ObjectNormalizer. We already have a suggest entry in the composer.json file, though).To me option 2 looks better. WDYT?Commits-------d297e27600 [FrameworkBundle] Do not load property_access.xml if the component isn't installed
Uh oh!
There was an error while loading.Please reload this page.