-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
[JsonEncoder] Add component documentation #20486
base: 7.3
Are you sure you want to change the base?
Conversation
f08a11e
to
592f9aa
Compare
592f9aa
to
afa08c8
Compare
The remaining failure is due to the fact that the https://github.com/symfony-tools/symfony-application application does not have the Should we create it? Or should we add the issue to the baseline? Or maybe commenting that configuration? |
|
||
public function load(string $className, array $options = [], array $context = []): array | ||
{ | ||
$propertyMetadataMap = $this->decorated->load($className, $config, $context); |
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.
$propertyMetadataMap = $this->decorated->load($className, $config, $context); | |
$propertyMetadataMap = $this->decorated->load($className, $options, $context); |
because the data is processed incrementally rather than loading the entire | ||
file into memory. | ||
|
||
To improve memory efficiency, the JsonEncoder creates _`ghost objects`: https://en.wikipedia.org/wiki/Lazy_loading#Ghost |
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.
To improve memory efficiency, the JsonEncoder creates _`ghost objects`: https://en.wikipedia.org/wiki/Lazy_loading#Ghost | |
To improve memory efficiency, the JsonEncoder creates `ghost objects`_ |
And at the bottom of the file :
.. _ghost objects
: https://en.wikipedia.org/wiki/Lazy_loading#Ghost
Hi @mtarld. Thanks a lot for starting this and providing so much valuable documentation early in the development phase! Before reviewing the whole document, we must think about the position of the component in the documentation. We've mostly stopped using the I think the best way would be to move this article to Something that might also be interesting is adding a |
# config/packages/json_encoder.yaml | ||
framework: | ||
json_encoder: | ||
paths: # where the objects to be encoded/decoded are located | ||
App\Encodable\: '%kernel.project_dir%/src/Encodable/*' |
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 we think about adding this config + the src/Encodable/
direction to a recipe for symfony/json-encoder
? Or is there a common use-case of JsonEncoder where you wouldn't want this?
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.
Sure, I think that this recipe will be really useful!
About the Encodable
name, do you think it is clear enough, or should we find another name?
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.
I think I was slightly confused here (and only noticed it when you asked about the name). This config option configures where our DTOs live, so we can generate the code at warmup instead of runtime? That might be a bit too application-specific to include in a recipe (would be cool if we could do auto discovery based on a PHP attribute or something, but that's a mechanism we do not yet have for non-services atm).
If I'm understanding this correctly, I would also suggest moving this slightly more down in the documentation. Let's first show people the high level usage, so they understand how the component works, and then suggest they pre-generate the required PHP code.
I guess that will also fix the build error, as src/Dto/
exists further down in the article (because we create the Cat
example entity).
(btw, I'm planning on reading the whole article this week, depending on how much work there is in the office 😄 )
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.
Yes you're totally right, let's move the "cache generation" section a bit later 🙂
Fixes #20461