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

Add compatibility with named parameters and constructor property promotion #369

Merged
merged 5 commits into from
Oct 17, 2020

Conversation

beberlei
Copy link
Member

With PHP 8 the combination of named parameters and constructor property
promotion will be very cool for the new Attributes feature, but also for
Doctrine's Annotation feature. Especially if classes are to be used
for both Attributes and Annotations, then the DocParser needs
a compatible support for named parameters in annotation class constructors.

For backwards compatibility this is done with a marker interface that
uses a different instantiation logic than the current one. Currently
all annotations properties are passed as a single array argument
$values.

This patch uses reflection to match annotation values to class
constructor parameters. For PHP 8 the integration of named arguments
with spread operator is used.

A PHP 8 class that could be read using Doctrine Annotations would
become:

/** @Annotation */
class Table implements NamedArgumentConstructorAnnotation
{
    public function __construct(
        public string $name,
        public array $indexes = []
    ) {}
}
/** @Table(name="users") */
/** @Table(name="users", indexes={"foo"}) */

greg0ire
greg0ire previously approved these changes Oct 12, 2020
Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

I think this should target master, since it's not a bugfix.
Also, should docs be added regarding the new feature and how to migrate to it?

@greg0ire greg0ire dismissed their stale review October 12, 2020 11:46

Didn't mean to approve, sorry.

Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

I think this should target master, since it's not a bugfix.
Also, should docs be added regarding the new feature and how to migrate to it?


/**
* Marker interface for PHP7/PHP8 compatible support
* for named arguments (and constructor property promotion).
Copy link
Member

Choose a reason for hiding this comment

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

Should constructor property promotion really be mentioned here? You did not write a test for it, so I believe this means it does not really come into play here: it will work the same regardless of whether you use that feature or not.

…otion

With PHP 8 the combination of named parameters and constructor property
promotion will be very cool for the new Attributes feature, but also for
Doctrine's Annotation feature. Especially if classes are to be used
for both Attributes and Annotations, then the DocParser needs
a compatible support for named parameters in annotation class constructors.

For backwards compatibility this is done with a marker interface that
uses a different instantiation logic than the current one. Currently
all annotations properties are passed as a single array argument
$values.

This patch uses reflection to match annotation values to class
constructor parameters. For PHP 8 the integration of named arguments
with spread operator is used.

A PHP 8 class that could be read using Doctrine Annotations would
become:

/** @annotation */
class Table implements NamedArgumentConstructorAnnotation
{
    public function __construct(
        public string $name,
        public array $indexes = []
    ) {}
}
/** @table(name="users") */
/** @table(name="users", indexes={"foo"}) */
@beberlei beberlei force-pushed the NamedParametersSupport branch from c0177d9 to c2af93a Compare October 12, 2020 21:25
@beberlei beberlei changed the base branch from 1.10.x to master October 12, 2020 21:25
@beberlei
Copy link
Member Author

@greg0ire right, rebased on master. I will document this as soon as we decide that this makes sense.

@beberlei beberlei requested a review from ostrolucky October 13, 2020 08:00
docs/en/custom.rst Outdated Show resolved Hide resolved
@beberlei beberlei merged commit 65cf8ab into doctrine:master Oct 17, 2020
@beberlei beberlei deleted the NamedParametersSupport branch October 17, 2020 20:37
@beberlei beberlei added this to the 1.11.0 milestone Oct 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants