-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[AstGenerator] [WIP] New Component, add normalizer generator #17516
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
[AstGenerator] [WIP] New Component, add normalizer generator #17516
Conversation
$nodes = []; | ||
|
||
foreach ($this->generators as $generator) { | ||
if ($generator instanceof AstGeneratorInterface && $generator->supportsGeneration($object)) { |
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.
The $generator instanceof AstGeneratorInterface
test should be moved in the constructor to avoid doing it at every call to generate()
What about making the AstGenerator and the normalizer generator two separate components? I'm very enthusiast about the generic AstGenerator and I think about a lot of usages for it. However, I'm less convinced by the normalizer generator:
|
Don't want to add too many components for generator, but i was also wondering if generator about a specific component should be directly in the component (and then we can propose AstGenerator as a suggested dependency ) or if all generators are inside AstGenerator ?
Same, the way i write AstGeneratorInterface is, IMO, enough generic to be usable in other places.
I agree it may be better but not sure and i would like to add some benchmarks to this, is there already a place with some benchmarks in tests or a prefered way to do this ? Also don't forget that we cab also add ast optimization (by adding a visitor layer on top of the generation) which can improve performance even more.
My intent is more to generate the normalizer into cache directory for the Framework and then in dev environment it will generate each time (generation is enough fast for dev IMO)
Yes there is many use case to handle, and want to keep the scope small for a first preview, but i definitely want to add them. However i don't want to offer the possibilty to edit generated normalizer. To add custom code to the normalizer the way would be to add a custom generator by using the AstGeneratorChain class. I also think we can heavily drop the maintenance by mutualising logic of all this use case for generators and for existing class. Maybe the normalizer generator is a scope too large for a first preview of this component, do you have something in mind for reduced implementation as a first example of this generator ? For me components that can profit of this (not exhaustive list) :
|
@joelwurtz thanks for proposing a new component for Symfony. Although @dunglas's comments are interesting, I recommend you to stop working on this pull request until a decision is made about integrating this component in Symfony or not. It would be a shame to make you waste time on something that ultimately won't be merged. |
@javiereguiluz We've talked about this new component with @joelwurtz and @dunglas in the original discussion linked in the description of this PR. |
Also @javiereguiluz most of the code comes from an existing library, so there is no so much work involved. The point is @dunglas and me have use case for generating code from existing, but different, schema. One of the objective was to mutualise the generation of code by using a common lib base on the PropertyInfo component. So i will definitely write something like this, having it part of symfony is a proposition :) To bounce back on @dunglas comments:
By giving more thoughts i think my answer was not correct, actually for most of this features, this generator doesn't care:
I will try to make some sorts of summary of all existing features of normalizer and show what need to be done in a more concret way. However i do not think there is much work to do here, AST can be scary when you have never do this, but in reality it's relatively easy, it's just very verbose :) |
the hydration part looks a bit like https://github.com/Ocramius/GeneratedHydrator |
It's close but not really the same, https://github.com/Ocramius/GeneratedHydrator hydrate the object by binding a generated closure to the model (so it can write to private / protected properties), here we only write / read if the property is public or a method is accessible (no magic hydration) also the hydration is not binded to the model class. |
To clarify my view on this component, here is more explanations Goal for a generic AstGenerator ComponentOne of the goal, despite generated normalizers, is to mutualise and share code generation in Symfony with a better approach than templating (replace, in the long term, the router dumper by example) Workflow
AstGenerator Internal workflowFor the generation part the following thing need to be achieved:
Runtime workflowThen at runtime the following thing will happen:
I believe this will allow to have a common workflow for generated code in symfony, also as there will be a unique pipeline for generated code, many tools can be added without duplication:
I hope this give a better vision of this component. |
Nice summary! 👍 for centralizing the code generation across Symfony. |
The idea seems neat, but the Shouldn't you dump a |
Hey @Ocramius nice to you see there !
Your are talking about renaming the Interface to The analyze part is not done in this repository as it's the reponsability of the others components which extract metadata. |
* | ||
* @return \PhpParser\Node[] An array of statements (AST Node) | ||
*/ | ||
public function generate($object, array $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.
Don't you think that we should expect a clear contract here (object *Context vs array)
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.
arrays are very flexible and have proven to be very good extension points in Serializer and PropertyInfo components. I'm in favor of keeping this as is.
@joelwurtz What do you think about rewriting generators provided by SensioGeneratorBundle with this new component as a "demo" of its possibilities? |
I strongly endorse it. I tried various generator approaches, and moving to AST-only caused an explosion in complexity for my use-cases (which is why I kept using |
Regarding generic AstGenerator component, AST node manipulation, is quite complex and also really verbose. It suits well compilers and transpilers, however, for this use-case I think it is much easier to use something like The problem I've always had with Symfony-generated classes is that they're treated only as a "cache". If new code generator component replacing router dumper etc. would be introduced, I would like to see ability to better manipulate output destination, so the generated code can be committed to VCS alongside the hand-written code. As for generated normalizer, I have implemented skrz/meta which does something similiar, however, its approach is generated-code-only - everything is generated ahead-of-time and generated code in runtime has almost none dependencies on the library. We saw great performance improvements compared to any kind of (however optimized) serialization code based on runtime instrospection (so although Serializer & PropertyAccess components might have seen great performance gains I bet generated code will always trump them), I'm looking forward to that Serializer component is moving to generated code. But once the normalizer/serializer code is being generated, there is really no use for normalization phase before serialization phase. skrz/meta generates specific serialization/deserialization code for every input/output format - currently JSON (because PHP doesn't have useful fast streaming parser/encoder for JSON, this serialization still needs to be piped through array object normalization), XML, and Protocol Buffers. I think generated serializers should also generate specific code for each format and skip normalization phase whatsoever. Also regarding recent emphasis on client-facing API contracts and that client-side should drive returned fields from the server (i.e. Facebook's GraphQL, or Netlix's Falcor), for Serializer component to stay relevant to current client-side needs, I think it's imperative to implement some kind of filtering for serialized object properties, |
Why |
class ArrayHydrateGeneratorTest extends \PHPUnit_Framework_TestCase | ||
{ | ||
/** @var Standard */ | ||
protected $printer; |
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.
this could be private.
Field-by-field filtering results in big response payload size reduction. |
They can nest: https://api-platform.com/doc/1.0/api-bundle/serialization-groups-and-relations#embedding-relations Field by field granularity can be easily implemented using groups: one group per field and a the controller create the context based on the query parameter. It can also be easily implemented (without |
What is locking here ? |
Nothing in particular, was waiting for some input and didn't have the time the past week to look into this, will try to respond to all of the questions and advance this in the end of this week |
Ok thank you a lot for your work @joelwurtz ☺ |
For @jakubkulhan
I disagree, this may be more complex for simple use case (however it's very subjective) but it's easier for complexe use case (like generating Normalizer), i have build this library https://github.com/jolicode/jane and my first implementation was using Zend\Code and similar library not dealign with Ast, and it was an absolute crap because i have to know when i have a set method, or only a object (not the same assignation), i needed to do many tiny templates files which where hard to debug. On the other hand working with AST is much simpler as everything is an Expr, so yes in some case it can bring bugs (as not every Expr can work with other Expr), but PHP-Parser was good enough for me to detect this bugs. Also working with AST you don't have to deal with formatting your code, only the logic part, and you can deal with formatting latter when printing your AST, so it's easier from this POV as it completly remove this concern when developing. Futhermore, i want to keep a centralized and unique approach for generating code in Symfony for better reusability across components, and i strongly believe that using PHP-Parser is a better choice than Zend\Code. In fact this library will be more a concurrent of the Zend\Code, but with a different approach : focusing on "functional" POV but not technical (i.e. there will be generators for normalization, hydration or others classic things, but no generators for methods / class / ...)
This is not the subject for this PR, and this decision should be in a futur PR, however yes it's a first step towards this. Also the purpose of having a well designed generic interface is to allow users injecting they own generator, so specific logic for some third party bundles (like in the router component) can also be generated.
Not so sure about this, only dealing with normalizer is best for me, as i don't have to think to the differents formats, also the (de)serialization part from/to simple array to/from json, xml, ... cannot be optimized IMO with the current library available from PHP.
I think just having conditions around each field would be enough (so if field present we normalize, otherwise we don't treat it), take a look at the generated code on this lib https://github.com/jolicode/jane-openapi it's close to what i intend to generate for normalizers. For @dunglas
Not sure if this is the right pick, as it's not present in this lib (and would induce more time for crossed dependencies work) My plan right know is to make a benchmark with the current component, the PR about optimisation on Serializer, and the generated code from this library https://github.com/jolicode/jane so we can have a idea if it's worth it, is that ok with you ? |
7fed221
to
de38c7b
Compare
It's kind of leaking to have the I think that as @joelwurtz wrote elsewhere, this should be moved into a subpart of the Serializer component, dedicated to generating normalizers. Using PhpParser should be handled as an internal implementation "detail" (thus tagging |
Yes, i agree, we should do a first internal implementation that works out of the box, then, if they are need, allow user / bundle customization. If i remember well @dunglas got this problem with the PropertyPath (or PropertyAccess ?) component, where there was too many things with public API and it was very hard to maintain BC on this. We should not repeat the same error. |
Not sure of what you mean. IIRC both PropertyAcess and PropertyInfo components don't use external dependencies in their APIs. |
Maybe the first version of the generator should be kind of hardcoded (i mean coupled to the serializer component) and the user not able to hook into it which shouldn't be a problem if we make it internal. |
Closing for housekeeping - don't hesitate to submit again when ready. |
This new component brings the concept of generating code in an AST representation into Symfony.
The original idea of doing it comes from the following discussion : api-platform/schema-generator#42
The first focus is to be able to generate
Normalizer
from existing class by extracting property and types with thePropertyInfo
Component, so the following class :will generate the following normalizer:
Why ?
There is some PR and discussion about making the serialization faster, i believe, by generating such a normalizer, the speed could not be faster as all the logic will be computed in the generation process and not a the runtime.
What is done
For the moment this library generate 2 things:
What is left
There is still some works to do, but this is a first preview so we can talk about it more without going too far, here is what's left from my point of view:
call
mode (like using the serializer or other services)type
variable)Issues to address
There is some issues than need to be adressed to have a robust generation:
Interface
This library declare a new interface
AstGeneratorInterface
it's enough generic to be used for each kind of generation and also with the usage ofAstGeneratorChain
it will be easy for user to hook into the generation process.Hydration
On the hydration part the plan is to be able to transform:
Even if
array
is the standard in the serializer component, stdClass is needed IMO, as it's the only possible solution to distinct between an empty object or an empty array in the JSON wrold ({}
vs[]
}Others ideas
I have plenty of ideas for this component but i want to keep the scope small at the moment and will do others PR: