Deprecate the behavior of the JMSHandlerRegistry to improve performance in v3 #2227
Conversation
|
I have talked to @xabbuh over slack about this:
I'm 👍 for this change, but I'm not familiar with what |
|
I think we should not block the 3.0 release with this PR and first discuss a bit what we want to achieve here. For example, would it make sense to add an option to completely disable our custom registries (in 3.1 then)? If I don't miss anything, they are not that useful anymore. |
You're right they could be completely removed, in 3.x the bundle only uses instances of I'll modify this PR accordingly then! |
|
👍 sounds like a good plan |
| $options['serializeNullStrategy'] = $config['serializer']['serialize_null']; | ||
| $viewHandler->addArgument($options); | ||
|
|
||
| if ($config['serializer']['disable_custom_jms_registry']) { |
There was a problem hiding this comment.
I'm not sure whether we should trigger a deprecation warning in case this option is not activated as it will create noise for the user but shouldn't make the removal of the custom registry safer (the deprecation triggered here ensures it has no effect).
Serializer/JMSHandlerRegistry.php
Outdated
| $handler = $this->registry->getHandler($direction, $typeName, $format); | ||
| if (null !== $handler) { | ||
| if (!$first) { | ||
| @trigger_error(sprintf('Relying on the custom registry %s to inherit the JMS handler of type `%s` is deprecated since FOSRestBundle 3.1. It will be removed in version 4.0. Use the option `fos_rest.serializer.disable_custom_jms_registry` to disable it.', __CLASS__, $typeName), E_USER_DEPRECATED); |
There was a problem hiding this comment.
Are we sure this deprecation will always be triggered? I could imagine that's not the case if $first never becomes false. I tend to think that triggering a deprecation when the option is set to false would be the better approach. Deprecating this class is not really necessary (at least not triggering a deprecation) as it is already marked as internal.
There was a problem hiding this comment.
If the deprecation is not triggered it is either because
- The handler returned is the one expected (the one returned by the jms registry)
- No handler is returned and the behavior is thus the same as the jms registry
I wanted to deprecate is relying on the behavior of this class, so as long as the result is the same as what would jms return I believe it's ok to not trigger a deprecation.
But if we want to warn users they could improve performance by disabling this class, triggering a deprecation when $first would indeed be legit.
So in my opinion it depends whether we want to warn users about the performance improvement possibility or not. I think it is better to privilege DX but I would appreciate having your opinion on this point :)
There was a problem hiding this comment.
I think we should always trigger the deprecation to make users aware of the performance penalty this may have.
|
The travis failure is quite strange, I don't think it's related to this PR. |
UPGRADING-3.0.md
Outdated
| * `FOS\RestBundle\View\View` | ||
| * `FOS\RestBundle\View\ViewHandler` | ||
|
|
||
| * The JMS Handlers `JMSHandlerRegistry` and `JMSHandlerRegistryV2` behaviors have changed. |
There was a problem hiding this comment.
@GuilhemN I guess this should be moved to the CHANGELOG instead since 3.0 has already been released
|
closing in favor of #2319 for jms/serializer-bundle v4 |
4 participants
Sorry I am quite late at submitting changes for v3...
I think it wasn't a good idea to introduce such an handler in the first place (sorry for that) and I think it should be reworked to do just what we want it to: inherit the FlattenException handler.
As @goetas noted (a while ago now) in #1899 (comment), it has terrible performance issues and I think v3 would be a great opportunity to fix it.
I hope it's still ok for you but if not, well I missed the boat :)