Skip to content

[VarExporter] add #[Ignore] to proxy-related methods to prevent them from being serialized #54224

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

Merged
merged 1 commit into from
Mar 19, 2024

Conversation

priyadi
Copy link
Contributor

@priyadi priyadi commented Mar 10, 2024

Q A
Branch? 6.4
Bug fix? yes
New feature? no
Deprecations? no
Issues
License MIT

Adds #[Ignore] to proxy-related methods so they are not included in serialization.

Previously, serializing a lazy object will result in something like this:

{
  "lazyObjectInitialized": false,
  "foo": "bar"
}

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Mar 12, 2024

I would prefer dealing with this on the Serialize side. Is that doable without too much impact?

@priyadi
Copy link
Contributor Author

priyadi commented Mar 12, 2024

It should be possible, but I don't know if there is any impact yet.

I considered that approach, but the consumer should not need to be aware it is dealing with a proxy object. However, either way it is going to be ugly. It is either Serializer needs to know it is dealing with a proxy or the proxy needs to tell Serializer to skip certain properties. Then, there are third party serializers and proxy generators.

To me, the best way forward is to change LazyObjectInterface. Instead of doing:

$object->isLazyObjectInitialized();

we do this:

$object::isLazyObjectInitialized($object);

Alternatively, it should offer the option not to expose LazyObjectInterface. Similar to what Doctrine does, but with better DX.

WDYT?

@nicolas-grekas
Copy link
Member

Yes I agree, it'd be better to move all methods of LazyObjectInterface outside of it. We'd need a BC-path forward though.

@nicolas-grekas
Copy link
Member

Why adding the attribute on e.g. lazyObjectState while only isLazyObjectInitialized leaks?

@priyadi
Copy link
Contributor Author

priyadi commented Mar 18, 2024

I added them for completeness, because some serializers access private properties directly.

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big fan of the concern leaking here but still pragmatic. The plan might be to remove this API in favor of a static one so fine by that plan also.

@nicolas-grekas nicolas-grekas force-pushed the fix/proxy-normalization branch from be22a32 to 7c2e848 Compare March 19, 2024 12:10
@nicolas-grekas
Copy link
Member

Thank you @priyadi.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants