[Serializer] ObjectNormalizer: throw if PropertyAccess isn't installed #24563

dunglas · 2017-10-15T17:29:45Z

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#24553
License	MIT
Doc PR	n/a

xabbuh · 2017-10-16T06:51:33Z

I think we should also not register the ObjectNormalizer as a service in the FrameworkBundle when the PropertyAccess component is not installed. We did that in several other places.

dunglas · 2017-10-16T07:38:28Z

@xabbuh if we do that, will not it be very hard to debug?

aivus · 2017-10-16T08:31:56Z

src/Symfony/Component/Serializer/Normalizer/ObjectNormalizer.php

@@ -33,6 +34,10 @@ class ObjectNormalizer extends AbstractNormalizer

    public function __construct(ClassMetadataFactoryInterface $classMetadataFactory = null, NameConverterInterface $nameConverter = null, PropertyAccessorInterface $propertyAccessor = null)
    {
+        if (!class_exists('Symfony\Component\PropertyAccess\PropertyAccess')) {
+            throw new RuntimeException('Unable to use ObjectNormalizer as the Symfony PropertyAcess component is not installed.');


PropertyAccess

BoShurik · 2017-10-16T09:36:57Z

@dunglas otherwise impossible to use

composer create-project symfony/skeleton
composer req serializer

weaverryan · 2017-10-16T11:08:06Z

src/Symfony/Component/Serializer/Normalizer/ObjectNormalizer.php

@@ -33,6 +34,10 @@ class ObjectNormalizer extends AbstractNormalizer

    public function __construct(ClassMetadataFactoryInterface $classMetadataFactory = null, NameConverterInterface $nameConverter = null, PropertyAccessorInterface $propertyAccessor = null)
    {
+        if (!class_exists('Symfony\Component\PropertyAccess\PropertyAccess')) {
+            throw new RuntimeException('Unable to use ObjectNormalizer as the Symfony PropertyAccess component is not installed.');


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.

Good idea, however it will be many other texts to change (I've adapted an existing one, coming from FrameworkBundle IIRC).

Done, I've just added extra quotes around symfony/property-access.

dunglas · 2017-10-16T21:38:14Z

@xabbuh, after double checking ObjectNormalizer is the only generic normalizer registered. It has a low property: it is called only if no other normalizer is matching. I think it's better in term of DX to get this exception than a silent fail like "no normalizer if able to handle "Foo\Bar".

fabpot · 2017-10-16T22:17:15Z

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  | BC breaks? | no | Deprecations? | no  | Tests pass? | yes | Fixed tickets | #24553 | License | MIT | Doc PR | n/a Commits ------- 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  | Bug fix? | yes | New feature? | no  | BC breaks? | no | Deprecations? | no  | Tests pass? | no | Fixed tickets | #...  | License | MIT | Doc PR | n/a Similar to #24563 but for the Yaml encoder (not the same branch). Commits ------- 7d21caf [Serializer] YamlEncoder: throw if the Yaml component isn't installed

BoShurik · 2017-10-18T10:47:07Z

@dunglas this exception does not rise when using flex. Is it ok? I've got an exception when trying to access serializer component

PHP Fatal error:  Uncaught Symfony\Component\Debug\Exception\ClassNotFoundException: Attempted to load class "PropertyAccessor" from namespace "Symfony\Component\PropertyAccess".
Did you forget a "use" statement for another namespace? in /Users/boshurik/Projects/symfony-property-access-issue/var/cache/dev/srcDevDebugProjectContainer.php:435

ogizanagi · 2017-10-18T15:10:38Z

See #24605

@xabbuh

…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  | Bug fix? | yes | New feature? | no  | BC breaks? | no | Deprecations? | no  | Tests pass? | yes | Fixed tickets | #24563 (comment)  | License | MIT | Doc PR | N/A This PR actually aims to fix #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

@xabbuh

…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  | Bug fix? | yes | New feature? | no  | BC breaks? | no | Deprecations? | no  | Tests pass? | yes | Fixed tickets | symfony/symfony#24563 (comment)  | License | MIT | Doc PR | N/A This PR actually aims to fix symfony/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

[Serializer] ObjectNormalizer: throw if PropertyAccess isn't installed

184e574

carsonbot added Status: Needs Review Serializer Bug BC Break labels Oct 15, 2017

dunglas mentioned this pull request Oct 15, 2017

[Serializer] YamlEncoder: throw if the Yaml component isn't installed #24565

Closed

dunglas removed the BC Break label Oct 15, 2017

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 16, 2017

javiereguiluz approved these changes Oct 16, 2017

View reviewed changes

aivus reviewed Oct 16, 2017

View reviewed changes

weaverryan reviewed Oct 16, 2017

View reviewed changes

Update exception message

3bdbade

dunglas force-pushed the pa_exception branch from b5dc668 to 3bdbade Compare October 16, 2017 21:35

weaverryan approved these changes Oct 16, 2017

View reviewed changes

fabpot approved these changes Oct 16, 2017

View reviewed changes

fabpot closed this Oct 16, 2017

ogizanagi mentioned this pull request Oct 18, 2017

[FrameworkBundle] Do not load property_access.xml if the component isn't installed #24605

Closed

fabpot mentioned this pull request Oct 30, 2017

Release v3.4.0-BETA2 #24761

Merged

fabpot mentioned this pull request Oct 30, 2017

Release v4.0.0-BETA2 #24762

Merged

This was referenced Nov 10, 2017

Release v2.7.36 #24914

Merged

Release v2.8.29 #24915

Merged

Release v3.3.11 #24916

Merged

dunglas mentioned this pull request Nov 27, 2017

[Serializer] Fatal error: Class 'Symfony\Component\PropertyAccess\PropertyAccess' not found #20588

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Serializer] ObjectNormalizer: throw if PropertyAccess isn't installed #24563

[Serializer] ObjectNormalizer: throw if PropertyAccess isn't installed #24563

dunglas commented Oct 15, 2017 •

edited

Loading

xabbuh commented Oct 16, 2017

dunglas commented Oct 16, 2017

aivus Oct 16, 2017

BoShurik commented Oct 16, 2017

weaverryan Oct 16, 2017

dunglas Oct 16, 2017

dunglas Oct 16, 2017

dunglas commented Oct 16, 2017

fabpot commented Oct 16, 2017

BoShurik commented Oct 18, 2017

ogizanagi commented Oct 18, 2017

[Serializer] ObjectNormalizer: throw if PropertyAccess isn't installed #24563

[Serializer] ObjectNormalizer: throw if PropertyAccess isn't installed #24563

Conversation

dunglas commented Oct 15, 2017 • edited Loading

xabbuh commented Oct 16, 2017

dunglas commented Oct 16, 2017

aivus Oct 16, 2017

Choose a reason for hiding this comment

BoShurik commented Oct 16, 2017

weaverryan Oct 16, 2017

Choose a reason for hiding this comment

dunglas Oct 16, 2017

Choose a reason for hiding this comment

dunglas Oct 16, 2017

Choose a reason for hiding this comment

dunglas commented Oct 16, 2017

fabpot commented Oct 16, 2017

BoShurik commented Oct 18, 2017

ogizanagi commented Oct 18, 2017

dunglas commented Oct 15, 2017 •

edited

Loading