[3.0] Use serializer expression language #267
Conversation
|
Looks good so far 👍 Maybe it is a good time to add phpstan with the strict rules ? |
|
@adrienbrault Did not think about it, but sounds a great idea! |
|
I have updated the description adding a proposal for some changes. None of them are implemented yet, I'm open to hear feedback! |
Good idea, but it should be done in a separate PR! |
| @@ -13,21 +13,19 @@ | |||
| } | |||
| ], | |||
| "require": { | |||
| "php": "^5.5|^7.0", | |||
| "php": "^7.2", | |||
There was a problem hiding this comment.
Why not 7.1? Are you using any specific language feature?
There was a problem hiding this comment.
jms/serializer requires 7.2, and this lib fully depends on it
| @@ -13,21 +13,19 @@ | |||
| } | |||
| ], | |||
| "require": { | |||
| "php": "^5.5|^7.0", | |||
| "php": "^7.2", | |||
| "doctrine/common": "~2.0", | |||
There was a problem hiding this comment.
Common is deprecated and splitted into several libraries, we should drop it.
There was a problem hiding this comment.
Hmm, true. Will check where is used. Probably in a separate pr
|
@willdurand @adrienbrault what are your toughs on
|
|
@goetas Would there be a way for a library/application to add relations at compile time to a class ? |
|
Actually should be not too complicated to move |
|
@goetas Would there also be an easy to add serializer type configuration to embedded ? The goal is that the api doc bundle could then show sample models/schemas within |
This is something that at the moment this lib does not offer, but is something I definitivnim plan to add (i have exactly the same use-case!) |
|
@goetas Ok ;-). Just asking in case it's easier to do in the refactoring, though it's probably better as a follow up PR |
| @@ -34,5 +35,11 @@ public function __construct(Exclusion $exclusion = null, Relation $relation = nu | |||
| $this->sinceVersion = $exclusion->getSinceVersion(); | |||
| $this->untilVersion = $exclusion->getUntilVersion(); | |||
| $this->maxDepth = $exclusion->getMaxDepth(); | |||
|
|
|||
| if ($exclusion->getExcludeIf() !== null && preg_match(self::EXPRESSION_REGEX, $exclusion->getExcludeIf(), $matches)) { | |||
There was a problem hiding this comment.
This logic is now duplicated 3 times. Why did you remove the ExpressionEvaluator class ?
There was a problem hiding this comment.
indeed, this will be solved in a separate PR (see #270)
| } else { | ||
| $serializedEmbeddeds[$embedded->getRel()][] = $navigator->accept($embedded->getData(), null, $context); | ||
| } | ||
| } catch (NotAcceptableException $e) { |
There was a problem hiding this comment.
Would it make sense to log this error ?
There was a problem hiding this comment.
not really, in jms 2.0 NotAcceptableException is used to skip nulls , see https://github.com/schmittjoh/serializer/blob/ccb142ce8f1a3b034449e52238dee5bea07c18bb/src/JsonSerializationVisitor.php#L94
Following up on #266, here a proposed refactoring for a possible v3
Currently is still work in progress, I would like to drop some functionalities.
Hope to have some time in the next days to prepare a list of features would like to drop ( to make the code more maintainable)feedback is welcome
EDIT: list of features I would like to drop:
relvalueThis attribute is the "reason" for a link to exist. This reason should be one. A "dynamic reason" means that most likely need a different link. Having this property not-dynamic will make the life easier when documenting the api (or generation documentation)
is_absoluteroute valueSame as for the point n.1. A link should be either relative or absolute, having it dynamic, makes it more difficult for the consumer to know what to do.
Same as for the point n.1 and n.2. The list of attributes to expect IMO should be well defined.
Makes life easier for clients, for documentation.
relation_providersThis point is related to the previous 3 points. Essentially the main idea is to get rid of "dynamic" things and have relations more predictable.
Removing the dynamical parts will make this lib more performant and will allow in the future to compile many parts of this library, that currently is done at runtime ( as example implementing #201)