Skip to content
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

Configurable generator using custom node visitors #40

Closed
wants to merge 3 commits into from

Conversation

sandermarechal
Copy link

This PR is a possible alternative to #39. It allows customizing the generated hydrators by setting custom node visitors in the configuration.

As an extension to this, perhaps even the built-in HydratorMethodsVisitor could be made configurable in this way? That would allow a user to replace the HydratorMethodsVisitor with his own implementation.

@sandermarechal
Copy link
Author

My personal use-case for this is that I want to modify the hydrate and extract methods to support nesting hydrators.

/**
* @param \PhpParser\NodeVisitor[] $visitors
*/
public function setCustomVisitors(array $customVisitors)
Copy link
Collaborator

Choose a reason for hiding this comment

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

May you can use setCustomVisitors(\PhpParser\NodeVisitor ...$customVisitors)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure but Maybe can be a good ideia pass $customVisitors by constructors.

Copy link
Author

Choose a reason for hiding this comment

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

Splatting isn't available before PHP 5.6 and the minimum required version for this library is now PHP 5.4. But I could do a check that all array elements are NodeVisitor instances.

As for constructor injection, custom visitors are optional so I don't think these belong in the constructor. IMHO only required settings should go in the constructor.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm fine with bumping to 5.6, don't let it block you.
On Sep 25, 2015 11:58 AM, "Sander Marechal" notifications@github.com
wrote:

In src/GeneratedHydrator/Configuration.php
#40 (comment)
:

@@ -220,4 +225,20 @@ public function getClassNameInflector()

     return $this->classNameInflector;
 }
  • /**
  • \* @param \PhpParser\NodeVisitor[] $visitors
    
  • */
    
  • public function setCustomVisitors(array $customVisitors)

Splatting isn't available before PHP 5.6 and the minimum required version
for this library is now PHP 5.4. But I could do a check that all array
elements are NodeVisitor instances.

As for constructor injection, custom visitors are optional so I don't
think these belong in the constructor. IMHO only required settings should
go in the constructor.


Reply to this email directly or view it on GitHub
https://github.com/Ocramius/GeneratedHydrator/pull/40/files#r40407046.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What about __construct @Ocramius ?

Copy link
Owner

Choose a reason for hiding this comment

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

The configuration object is a pile of mud on purpose: it serves as
mini-factory for the entire lib.
On Sep 26, 2015 01:27, "Jefersson Nathan" notifications@github.com wrote:

In src/GeneratedHydrator/Configuration.php
#40 (comment)
:

@@ -220,4 +225,20 @@ public function getClassNameInflector()

     return $this->classNameInflector;
 }
  • /**
  • \* @param \PhpParser\NodeVisitor[] $visitors
    
  • */
    
  • public function setCustomVisitors(array $customVisitors)

What about __construct @Ocramius https://github.com/Ocramius ?


Reply to this email directly or view it on GitHub
https://github.com/Ocramius/GeneratedHydrator/pull/40/files#r40477394.

Copy link
Author

Choose a reason for hiding this comment

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

I added an interface check to the custom visitor setter. I didn't want to use splatting because it would change the setter signature. You'd expect an array with a setter name like setCustomVisitors. It also means that what you get back from the getter is what you can pass into the setter.

@flip111
Copy link

flip111 commented Nov 11, 2015

So if i understand correctly ... when you have a bunch of custom visitors you decide the order on which you want to apply them by putting them in the right order in that array. What if you want to visit before the HydratorGenerator has ran? Maybe you want to do:

  1. customVistor1
  2. HydratorGenerator
  3. customVisitor2
  4. customVisitor3

About your use case .. you want to modify the proxy so that when it encounters an object you will execute another hydrator-proxy for that object? Which methods do you use to detect that a property on an object is meant for an object?

@flip111 flip111 mentioned this pull request Nov 11, 2015
@sandermarechal
Copy link
Author

@flip111 Running custom visitors before the HydratorGenerator is currently not supported by this PR at the moment. But it could be reworked to have the HydratorGenerator simply be another custom visitor that is always loaded by default.

As for my use case, I scan the docblock of every property. If it is documented with an @var and it is a valid classname, it executes a nested hydrator (using a Doctrine instantiator to create a new object for that property)

@sandermarechal
Copy link
Author

The only problem I have encountered so far is with the hydrator classname generator. At the moment it only uses the factory instance as a hash value, but it should also use the list of custom hydrators. At the moment, you cannot generate two hydrators for the same class using different custom visitors. They both end up having the same classname.

Just adding the custom visitors in the hash is not possible now. The hash becomes too long and the class/filenames become invalid. To solve this, the hashing and verification method from the ProxyManager project should be ported first (or split off into a separate library used by both GeneratedHydrator and ProxyManager)

@flip111
Copy link

flip111 commented Nov 26, 2015

@sandermarechal i'm trying to tackle the long filename problem from multiple angles. Your thoughts on these solutions would be appreciated .. i'm refering to file

https://github.com/flip111/GeneratedHydrator/blob/patch-2/src/GeneratedHydrator/Factory/HydratorFactory.php#L61 of PR https://github.com/Ocramius/GeneratedHydrator/pull/42/files

and

Ocramius/CodeGenerationUtils#5

@Ocramius Ocramius self-assigned this Jan 12, 2016
@Ocramius
Copy link
Owner

Looked at this again: won't fix, as I don't think the behavior should be configured to this detail level.

Instead, a new factory should be created.

@Ocramius Ocramius closed this Jan 12, 2016
@Ocramius
Copy link
Owner

@sandermarechal feel free to poke me - I am interested in the original use-case, I just don't see this as the correct approach

@sandermarechal
Copy link
Author

@Ocramius No need. You merged #39 which allows me to override the HydratorGenerator. That's all I need for my extension points :-)

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

Successfully merging this pull request may close these issues.

4 participants