-
-
Notifications
You must be signed in to change notification settings - Fork 586
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
Exclude properties based on semver constraints #1361
base: master
Are you sure you want to change the base?
Conversation
79bc13e
to
62d92f6
Compare
be9c84f
to
8e50c6b
Compare
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.
Thanks @W0rma for this PR! I left two minor comments.
Do you think that we should keep Until
and Since
attributes or they should be deprecated?
…sion constraints accepted by composer
8e50c6b
to
c2a0e19
Compare
@scyzoryck Good point. I think they should be deprecated since version constraints provide even more flexibility. Do you agree? |
Thanks. Looks good to me.
It was my initial thought - as basically new constraint covers all functionalities provided by older constraints. On the other hand new one requires external dependency, so keeping simpler constraints might made sense too. @goetas - what do you think about it? |
@@ -57,20 +57,21 @@ expose them via an API that is consumed by a third-party: | |||
class VersionedObject | |||
{ | |||
/** | |||
* @Until("1.0.x") |
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 would not abandon @Until
and @Since
, mainly for performance reasons. my guess is that composer/semver
is way slower that the php's version_compare
function, in a large object graph in it can make the difference.
I think that it could be also mentioned in the documentation the potential performance impact.
BTW, i would be curious to see a benchmark about it
{ | ||
public function __construct($values = [], ?string $version = null) | ||
{ | ||
if (!class_exists(Semver::class)) { |
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 if this check is done in the metadata drivers? so we avoid to call it at runtime each time the annotation is instantiated?
This PR adds the possibility to exclude properties from serialization based on version constraints accepted by composer.
Example:
I'm not sure if
VersionConstraints
is a good name or if there are better alternatives. Suggestions are welcome :)