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

Visitor interfaces in handlers #1129

Merged
merged 1 commit into from
Nov 8, 2019

Conversation

derzkiy
Copy link
Contributor

@derzkiy derzkiy commented Sep 30, 2019

Q A
Bug fix? no
New feature? yes
Doc updated yes/no
BC breaks? no
Deprecations? no
Tests pass? no
Fixed tickets
License MIT

When we need override visitor we also need overriding all handlers. Can we use visitor interfaces instead final class?

@derzkiy derzkiy force-pushed the fix-date-time-handler branch from bd81634 to c2291ed Compare September 30, 2019 16:17
@goetas
Copy link
Collaborator

goetas commented Oct 1, 2019

I understand the need of this PR, but handlers ofthen have visitor specific code (see

return $visitor->visitSimpleString($date->format($this->getFormat($type)), $type);
as example).

In my experience overriding visitors is often a bad idea... can you share the reasons why you are doing that? Maybe I can suggest a better solution...

@derzkiy
Copy link
Contributor Author

derzkiy commented Oct 2, 2019

For example, before updating serializer, i could override visitor and modified field value (trim, apply some purifiers) and returned null value if it was empty.

And your example uses an interface instead class.

But I will be glad to know the properly solution

@goetas
Copy link
Collaborator

goetas commented Oct 2, 2019

For example, before updating serializer, i could override visitor and modified field value (trim, apply some purifiers) and returned null value if it was empty.

I would suggest using type handlers for that. Something as:

/**
 * @JMS\Type("TrimmedString")
 */
protected $name;

And then add a type handler for TrimmedString. In this way you do not need to change too many parts in the serialization process.

@anyx
Copy link
Contributor

anyx commented Oct 2, 2019

@goetas using handlers for this case looks strange in my opinion: if we want to purify every string field in project, we need to add this type for every field (and override other custom handlers) for every property in project's models.

I agree that overriding visitors is often a bad idea, but I think, sometimes it's not. Anyway, it should be library user's responsibility. Now we forbid it for users (and for Barbara Liskov specially))

@derzkiy
Copy link
Contributor Author

derzkiy commented Oct 2, 2019

I think its not a better solution if you need apply modifiers to all fields in all models or dto.

If you want to teach community use composition over inheritance, its good. But you have to be consistent and use SOLID principles not so selectively.

@goetas goetas merged commit c2291ed into schmittjoh:master Nov 8, 2019
@goetas
Copy link
Collaborator

goetas commented Nov 8, 2019

Thanks for this PR. I did a small change in 95765aa because some handlers depends explicitly on some visitor implementations

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