Default property with @NamedArgumentConstructor #402

Vincz · 2021-01-19T09:16:07Z

Use the first constructor argument as default property when @NamedArgumentConstructor is used.

stof · 2021-01-19T09:22:35Z

docs/en/custom.rst

@@ -100,7 +100,8 @@ you can simplify this to:
    }

 Alternatively, you can annotate your annotation class with
-``@NamedArgumentConstructor`` in case you cannot use the marker interface.
+``@NamedArgumentConstructor`` in case you cannot use the marker interface. 
+In this case, the first argument of the constructor will be considered as the default property. 


The way this is written makes me think that this works only when using the @NamedArgumentConstructor annotation but not when using the marker interface, as you added that in the paragraph about the alternative. Is it actually true ?

It is true. It is the intended behaviour as we didn't want to introduce breaking change with current implementation of the interface.
See #396

Vincz · 2021-01-26T12:58:43Z

Hey guys :) Any feedback about this?
ping @greg0ire
ping @beberlei
ping @derrabus

greg0ire · 2021-01-27T19:45:02Z

I see @derrabus mentioned that we should deprecate the interface, and I quite agree, I'd rather not have 2 ways of doing the same thing, with differences that are hard to grasp (the only comment on this PR so far is about exactly that BTW). Did you overlook that or is it planned for later?

Vincz · 2021-01-29T15:23:41Z

@greg0ire Ok, the interface has been deprecated and the doc updated.

greg0ire · 2021-01-30T18:08:14Z

Please kindly squash your commits together. Maybe not all, but some definitely needs to be squashed. If you don't, we'll try to remember to do it for you but it's best if you save us this trouble.

How to do that?

git rebase -i origin/1.12.x, assuming origin is a git remote that points to this repository, and not your fork. If you're not sure what your remotes are, run git remote -vvv, there should be your fork and the holy/reference/base/origin/whatever-you-call-it repository.
A window will show up with many lines, replace pick with fixup on every line but the first one
Close your editor, git should do its magic, and you should end up with one commit
Use git push --force to overwrite what you already push. Don't forget the --force option otherwise git will try to merge both things together.

Use the first constructor argument as default property when @NamedArgumentConstructor is used

Vincz · 2021-01-31T08:03:20Z

@greg0ire Done!

greg0ire · 2021-02-05T07:34:12Z

ping @stof @beberlei 🙂

beberlei

An idea to improve the patch a little to be less invasive.

beberlei · 2021-02-05T21:54:59Z

lib/Doctrine/Common/Annotations/DocParser.php

@@ -859,7 +863,17 @@ private function Annotation()
            );
        }

-        $values = $this->MethodCall();
+        $defaultProperty = 'value';


Instead of this code block and passing down $defaultProperty could you instead handle this logic in the original line 944 and following?

foreach ($values as $property => $value) { if ($property === 'value') { $property = self::$annotationMetadata[$name]['default_property']; }

That would be less invasive and keep the logic at fewer places. I am not 100% sure it could work this way though, just a rough idea.

Hey @beberlei! Thx for the review. I didn't want to pass the defaultProperty down. The problem is, without it, we don't know with the return of values(), if value as been set because it's the default property... or because we have an argument named
value. So it would not work if for example the constructor looks like this:

public function __construct(string $name, int $value){}

But isnt that already a problem? Or just for the new named constructors?

I gave a bunch of examples here : #396.
The fact that we can't know if value has been set because it's the default property or because we have an argument with the same name, is, indeed an "old problem".
But we didn't want to change the current behaviour to avoid BC breaks, so we decide to handle it properly only with the new @NamedArgumentsConstructor annotation as it is not published yet.

The Problem is all methods with large letter starting are named after the rules in the grammer. None of them accept arguments, because they already operate on state of the class instead. Passing arguments now will create a huge inconsistency in the API.

What you could try instead is change the return value of Values to be an associative array with: ['named_arguments' => [], 'positional_arguments' => []] and then modify Annotation to act accordingly.

@beberlei Here is my first attempt trying what you suggested.
I was also wondering why this is allowed: @Name(@Name, @Name), but this is not @Name("1", "2"). The second case is not allowed because of this line:

annotations/lib/Doctrine/Common/Annotations/DocParser.php

Line 1037 in b81688b

throw $this->syntaxError('Value', $token);

In our case, it means that we can only use one positional argument. It would make sense for us to allow multiple positional arguments as it's also how attributes work. #[Name("1", "2")] would work for example as long as the related attribute constructor accept at least two arguments.
Removing the above line don't break the tests and allow multiple positional arguments.
What do you think about this?

In our case, it means that we can only use one positional argument. It would make sense for us to allow multiple positional arguments as it's also how attributes work.

Very well, but Doctrine Annotations only supports one positional argument: the default property. I would consider it out of scope of this PR to introduce the ability to have multiple ones.

Besides, a developer using an annotation does not know whether it has been implemented with a named constructor or not (and they should not need to know, imho). It would be kind of odd if multiple positional arguments only worked in the named argument constructor case.

Anyway, I'm a bit late to the party and I see that the discussion has already moved on. I'm fine with allowing multiple positional arguments. Thank you for working on this feature. 😃

@derrabus : Actually, Annotations already support multiple positional arguments. It's just that without @NamedArgumentConstructor all positional values will be populated as an array into value. This @Name(@Name, @Name) already work. So in a developer point of view, it doesn't really change anything.

beberlei

Much better! Only some small things left from my pov, then good to merge

beberlei · 2021-02-07T08:24:00Z

lib/Doctrine/Common/Annotations/DocParser.php

+            }
+        } else {
+            $values = $namedArguments;
+            if (! empty($positionalArguments) && ! isset($values['value'])) {


Please use count instead, empty does not communicate intent well

Isn't count() > 0 much less effective than !empty() ?

Yes, but the output of this code is usually cached and even if not, this is not a tight loop

I wanted to toy with phpbench so I build a little benchmark from this raw benchmark:

<?php class ArrayCheckerBench { public function provideArrays(): \Generator { yield 'empty array' => ['array' => []]; yield 'huge array' => ['array' => range(0, 100000)]; } public function checkArrayWithEmpty(array $x): bool { return empty($x); } public function checkArrayWithCount(array $x): bool { return count($x) === 0; } /** * @Revs(1000000) * @Iterations(5) * @ParamProviders({"provideArrays"}) */ public function benchCheckIfArrayIsEmptyWithEmpty(array $parameters): void { $this->checkArrayWithEmpty($parameters['array']); } /** * @Revs(1000000) * @Iterations(5) * @ParamProviders({"provideArrays"}) */ public function benchCheckIfArrayIsEmptyWithCount(array $parameters): void { $this->checkArrayWithCount($parameters['array']); } }

Here are the results:

vendor/bin/phpbench run ArrayCheckerBench.php --report=aggregate --retry-threshold=0.5 PhpBench @git_tag@. Running benchmarks. \ArrayCheckerBench benchCheckIfArrayIsEmptyWithEmpty # emp.I4 [μ Mo]/r: 0.285 0.285 (μs) [μSD μRSD]/r: 0.000μs 0.06% benchCheckIfArrayIsEmptyWithEmpty # hug.R1 I1 [μ Mo]/r: 0.287 0.287 (μs) [μSD μRSD]/r: 0.000μs 0.12% benchCheckIfArrayIsEmptyWithCount # emp.R1 I1 [μ Mo]/r: 0.309 0.309 (μs) [μSD μRSD]/r: 0.001μs 0.28% benchCheckIfArrayIsEmptyWithCount # hug.R3 I3 [μ Mo]/r: 0.311 0.311 (μs) [μSD μRSD]/r: 0.001μs 0.34% 2 subjects, 20 iterations, 4,000,000 revs, 0 rejects, 0 failures, 0 warnings (best [mean mode] worst) = 0.285 [0.298 0.298] 0.285 (μs) ⅀T: 5.958μs μSD/r 0.001μs μRSD/r: 0.201% suite: 134621f1b80d1bb3f7ed3eaf6bc32c7bb639d7d1, date: 2021-02-07, stime: 15:08:37 +-------------------+-----------------------------------+-------------+---------+-----+-------------+---------+---------+---------+---------+---------+--------+-------+ | benchmark | subject | set | revs | its | mem_peak | best | mean | mode | worst | stdev | rstdev | diff | +-------------------+-----------------------------------+-------------+---------+-----+-------------+---------+---------+---------+---------+---------+--------+-------+ | ArrayCheckerBench | benchCheckIfArrayIsEmptyWithEmpty | empty array | 1000000 | 5 | 526,184b | 0.285μs | 0.285μs | 0.285μs | 0.285μs | 0.000μs | 0.06% | 1.00x | | ArrayCheckerBench | benchCheckIfArrayIsEmptyWithEmpty | huge array | 1000000 | 5 | 64,194,552b | 0.287μs | 0.287μs | 0.287μs | 0.288μs | 0.000μs | 0.12% | 1.01x | | ArrayCheckerBench | benchCheckIfArrayIsEmptyWithCount | empty array | 1000000 | 5 | 526,184b | 0.308μs | 0.309μs | 0.309μs | 0.310μs | 0.001μs | 0.28% | 1.08x | | ArrayCheckerBench | benchCheckIfArrayIsEmptyWithCount | huge array | 1000000 | 5 | 64,194,552b | 0.310μs | 0.311μs | 0.311μs | 0.313μs | 0.001μs | 0.34% | 1.09x | +-------------------+-----------------------------------+-------------+---------+-----+-------------+---------+---------+---------+---------+---------+--------+-------+

Looks like count takes almost 10% more time, so in relative terms, it's far better indeed. In absolute terms though, it seems to be 30 ns slower… I don't think we should worry about such small differences.

beberlei · 2021-02-07T08:28:39Z

lib/Doctrine/Common/Annotations/DocParser.php

+        $positionalArguments = $arguments['positional_arguments'] ?? [];
+        $namedArguments      = $arguments['named_arguments'] ?? [];
+
+        if (


Could you extract this block into a private helper please?

$values = $this->assembleValues($arguments, $name);

Method name could be better

Vincz · 2021-02-07T14:54:25Z

I was also wondering why this is allowed: @Name(@Name, @Name), but this is not @Name("1", "2"). The second case is not allowed because of this line:

annotations/lib/Doctrine/Common/Annotations/DocParser.php

Line 1037 in b81688b

throw $this->syntaxError('Value', $token);

In our case, it means that we can only use one positional argument. It would make sense for us to allow multiple positional arguments as it's also how attributes work. #[Name("1", "2")] would work for example as long as the related attribute constructor accept at least two arguments.
Removing the above line don't break the tests suite and allow multiple positional arguments.
What do you guys think about this?

beberlei · 2021-02-07T14:57:51Z

@Vincz this only works for the new named argument ctor attributes though or? Because otherwise multiple values are handled as an array of value for the old Annotations or not? I would be fine supporting this for new annotations.

Vincz · 2021-02-07T15:13:05Z

I think it would allow @Named("foo", "bar") without the @NamedArgumentConstructor but it would set value as an array like ["foo", "bar"]. Currently it works if provided values are objects or array but not with scalar values (it raise a Syntax error).
With @NamedArgumentConstructor it would use the constructor arguments, ie. "foo" for the first one and "bar" for the second one.

beberlei · 2021-02-07T16:07:05Z

Ok

Vincz · 2021-02-07T18:29:30Z

Ok, I'll add a bunch of tests then and make sure it doesn't break anything.

Vincz · 2021-02-08T10:06:33Z

So, it is now possible to use @Name("foo", "bar") with regular annotation and we will get an array of string as explained.

With the @NamedArgumentConstructor we can now use multi positional values to initialise arguments.
I also added a check to ensure that it is not possible with @NamedArgumentConstructor to use @Annot("foo", bar=123, "baz") as it's an incorrect syntax with attributes / PHP 8 named arguments, and we want to mimic the behaviour.
I'm not sure about the raised exception type and message, let me know if you have a better idea.
Also, should I update the documentation to explain more deeply how @NamedArgumentConstructor work?

derrabus · 2021-02-09T09:23:46Z

lib/Doctrine/Common/Annotations/DocParser.php

+                    ($lastPosition === null && $position !== 0 ) ||
+                    ($lastPosition !== null && $position !== $lastPosition + 1)
+                ) {
+                    throw $this->syntaxError('Positional arguments after named arguments is not allowed');


Suggested change

throw $this->syntaxError('Positional arguments after named arguments is not allowed');

throw $this->syntaxError('Positional arguments are not allowed after named arguments');

derrabus · 2021-02-09T09:35:05Z

lib/Doctrine/Common/Annotations/DocParser.php

        }

-        return $values;
+        return ['named_arguments' => $namedArguments, 'positional_arguments' => $positionalArguments];


In resolvePositionalValues, you validate $positionalArguments. Can't we do this here already? I don't like that we pass around potentially invalid metadata.

The Values() method is part of the parsing process. If we move the validation inside, it will have to know what kind of annotation we are talking about. The validation is different is you use @NamedArgumentConstructor or not.
This is valid without @Name("1", foo="bar", "2") but should not be valid with @NamedArgumentConstructor as it's supposed to mimic a regular constructor call with named arguments.

All right, thank you for clarifying. 👍🏻

beberlei

Very good! I think we got a good state now that is ready to merge and then ship

kevinpapst · 2021-02-21T01:05:54Z

lib/Doctrine/Common/Annotations/DocParser.php

+        } else {
+            if (count($positionalArguments) > 0 && ! isset($values['value'])) {
+                if (count($positionalArguments) === 1) {
+                    $value = $positionalArguments[0];


stof reviewed Jan 19, 2021

View reviewed changes

Default prop with @NamedArgumentConstructor

9e2fe6d

Use the first constructor argument as default property when @NamedArgumentConstructor is used

greg0ire previously approved these changes Jan 31, 2021

View reviewed changes

greg0ire requested review from stof and beberlei January 31, 2021 09:10

greg0ire added the Improvement label Jan 31, 2021

greg0ire mentioned this pull request Jan 31, 2021

When will be released version 1.12? #403

Closed

beberlei mentioned this pull request Feb 5, 2021

[GH-8265] Prototype for Attribute Metadata Driver doctrine/orm#8266

Merged

5 tasks

beberlei requested changes Feb 5, 2021

View reviewed changes

Vincz dismissed greg0ire’s stale review via c404ba1 February 6, 2021 13:36

Values() will return an array of position

f1f293a

beberlei requested changes Feb 7, 2021

View reviewed changes

Move values resolution in dedicated helper

5c23889

Allow multi positional values

e917927

derrabus reviewed Feb 9, 2021

View reviewed changes

beberlei approved these changes Feb 9, 2021

View reviewed changes

beberlei merged commit ebec9b1 into doctrine:1.12.x Feb 14, 2021

Vincz mentioned this pull request Feb 16, 2021

[Bundle Split] Configuration loading overblog/GraphQLBundle#821

Open

greg0ire added this to the 1.12.0 milestone Feb 20, 2021

greg0ire added enhancement and removed Improvement labels Feb 20, 2021

kevinpapst reviewed Feb 21, 2021

View reviewed changes

beberlei mentioned this pull request Feb 28, 2021

Handle annotation default property #396

Closed

TomasVotruba mentioned this pull request Jul 9, 2021

Possible BC break in nested array annotations with @NestedAnnotation #418

Open

	throw $this->syntaxError('Positional arguments after named arguments is not allowed');
	throw $this->syntaxError('Positional arguments are not allowed after named arguments');

Default property with @NamedArgumentConstructor #402

Default property with @NamedArgumentConstructor #402

Conversation

Vincz commented Jan 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vincz commented Jan 26, 2021

greg0ire commented Jan 27, 2021

Vincz commented Jan 29, 2021

greg0ire commented Jan 30, 2021

Vincz commented Jan 31, 2021

greg0ire commented Feb 5, 2021

beberlei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beberlei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vincz commented Feb 7, 2021

beberlei commented Feb 7, 2021

Vincz commented Feb 7, 2021

beberlei commented Feb 7, 2021

Vincz commented Feb 7, 2021

Vincz commented Feb 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beberlei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment