Introduce annotation metadata #247

Majkl578 · 2019-02-23T09:13:53Z

Replace nested untyped metadata arrays structure inside DocParser by proper object metadata classes with builder.
This idea is somewhat inspired by #75 but this approach uses separate PropertyMetadata instead of plain untyped array as well as being more strictly typed.

Metadata will later be used by the new parser as well.
This is just an internals refactoring so it's not a BC break. No existing tests were touched.

Majkl578 · 2019-02-23T22:51:38Z

lib/Doctrine/Annotations/Metadata/PropertyMetadata.php

+     */
+    public function __construct(
+        string $name,
+        ?array $type,


Later on ?array will be replaced by proper Type.

Majkl578 · 2019-02-23T22:51:56Z

lib/Doctrine/Annotations/Metadata/PropertyMetadata.php

+        ?array $type,
+        bool $required = false,
+        bool $default = false,
+        ?array $enum = null


Later on enum info will be merged into Type.

lib/Doctrine/Annotations/Metadata/AnnotationTarget.php

lib/Doctrine/Annotations/DocParser.php

Ocramius

Overall looking good, what's the likeliness of this introducing a BC break, and should it target 1.x?

Ocramius · 2019-02-26T15:06:48Z

Also: needs a performance check now that #248 is merged

Majkl578 · 2019-02-26T18:02:16Z

This branch (already rebased):

\Doctrine\Performance\Annotations\DocLexerPerformanceBench

    benchMethod.............................I4 [μ Mo]/r: 7.067 7.086 (μs) [μSD μRSD]/r: 0.065μs 0.92%
    benchClass..............................I4 [μ Mo]/r: 10.108 10.017 (μs) [μSD μRSD]/r: 0.189μs 1.87%

\Doctrine\Performance\Annotations\DocParsePerformanceBench

    benchMethodParsing......................I4 [μ Mo]/r: 36.381 34.131 (μs) [μSD μRSD]/r: 4.568μs 12.56%
    benchClassParsing.......................I4 [μ Mo]/r: 32.458 31.741 (μs) [μSD μRSD]/r: 1.462μs 4.50%

\Doctrine\Performance\Annotations\PhpParserPerformanceWithoutShortCutBench

    bench...................................I4 [μ Mo]/r: 82.972 82.738 (μs) [μSD μRSD]/r: 0.507μs 0.61%

\Doctrine\Performance\Annotations\PhpParserPerformanceWithShortCutBench

    bench...................................I4 [μ Mo]/r: 88.515 88.243 (μs) [μSD μRSD]/r: 1.227μs 1.39%

\Doctrine\Performance\Annotations\ReadPerformanceBench

    bench...................................I4 [μ Mo]/r: 32.372 32.235 (μs) [μSD μRSD]/r: 0.314μs 0.97%

7 subjects, 35 iterations, 2,900 revs, 0 rejects, 0 failures, 0 warnings
(best [mean mode] worst) = 6.988 [41.410 40.884] 7.162 (μs)
⅀T: 1,449.365μs μSD/r 1.190μs μRSD/r: 3.260%

Master:

\Doctrine\Performance\Annotations\DocLexerPerformanceBench

    benchMethod.............................I4 [μ Mo]/r: 7.103 7.055 (μs) [μSD μRSD]/r: 0.093μs 1.31%
    benchClass..............................I4 [μ Mo]/r: 10.043 10.020 (μs) [μSD μRSD]/r: 0.044μs 0.44%

\Doctrine\Performance\Annotations\DocParsePerformanceBench

    benchMethodParsing......................I4 [μ Mo]/r: 23.252 23.183 (μs) [μSD μRSD]/r: 0.367μs 1.58%
    benchClassParsing.......................I4 [μ Mo]/r: 23.165 23.099 (μs) [μSD μRSD]/r: 0.148μs 0.64%

\Doctrine\Performance\Annotations\PhpParserPerformanceWithoutShortCutBench

    bench...................................I4 [μ Mo]/r: 83.942 83.336 (μs) [μSD μRSD]/r: 1.254μs 1.49%

\Doctrine\Performance\Annotations\PhpParserPerformanceWithShortCutBench

    bench...................................I4 [μ Mo]/r: 89.248 89.752 (μs) [μSD μRSD]/r: 1.226μs 1.37%

\Doctrine\Performance\Annotations\ReadPerformanceBench

    bench...................................I4 [μ Mo]/r: 23.663 23.359 (μs) [μSD μRSD]/r: 0.622μs 2.63%

7 subjects, 35 iterations, 2,900 revs, 0 rejects, 0 failures, 0 warnings
(best [mean mode] worst) = 7.030 [37.202 37.115] 7.276 (μs)
⅀T: 1,302.083μs μSD/r 0.536μs μRSD/r: 1.352%

Majkl578 · 2019-02-26T18:27:34Z

Overall looking good, what's the likeliness of this introducing a BC break, and should it target 1.x?

I wouldn't push it to 1.x at this state. It doesn't change public API (I reverted removal of Target::$literal) however it does remove the internal static annotations metadata registry - this makes i.e. ORM test suite slower (4.8s -> 5.3s). We could keep it static in 1.x though.

@alcaeus WDYT?

Ocramius · 2019-02-26T20:55:28Z

The bench for benchMethodParsing is quite worrisome though: 36% slower there.

Is there any hot path that we can inspect, or adding utility API on the builder to reduce hops?

alcaeus

Initial feedback below. In general I'd appreciate it if we can enable phpcs in travis-ci build pipeline before moving along with this. I've created #252 to track this.

In general, there are two main things I would like to see changed:

assert is used throughout for sanity checks. Since assertions can (and many times, are) turned off, I don't think this is feasible for checking input parameters. I believe it makes more sense to throw exceptions here.
The metadata builders are immutable. While I believe that metadata itself needs to be immutable, using the builder in DocParser feels cumbersome and would be drastically improved if they weren't immutable. See additional comment below.

lib/Doctrine/Annotations/DocParser.php

lib/Doctrine/Annotations/Metadata/AnnotationMetadata.php

alcaeus · 2019-02-27T06:38:34Z

lib/Doctrine/Annotations/Metadata/AnnotationMetadata.php

+            )
+        );
+
+        assert(count($defaultProperties) <= 1);


I wouldn't use assert here. To quote from the manual:

Assertions should not be used for normal runtime operations like input parameter checks. As a rule of thumb your code should always be able to work correctly if assertion checking is not activated.

I agree with that statement and would rather throw a subclass of InvalidArgumentException instead of using assert: while the code does not fail now if there is more than one default property, this can introduce issues that are difficult to debug at a later stage.

lib/Doctrine/Annotations/Metadata/AnnotationTarget.php

lib/Doctrine/Annotations/Metadata/MetadataCollection.php

alcaeus · 2019-02-27T06:49:15Z

lib/Doctrine/Annotations/Metadata/MetadataCollection.php

+     */
+    public function getIterator() : Traversable
+    {
+        yield from array_values($this->metadata);


Out of curiosity, why don't we return an iterator with keys?

I don't have strong opinion but it feels redundant to getName() and would encourage using $name => $metadata pattern.

alcaeus · 2019-03-05T13:06:16Z

As per #253 (comment), we've decided to not apply doctrine/coding-standard for now. @Majkl578 I've marked style comments in my review above as resolved, feel free to ignore that feedback unless it bothers you personally 😉

Majkl578 · 2019-03-05T15:03:17Z

Addressed review comments from @alcaeus. Also did two more changes:

removed ArrayAccess, it was superfluous and didn't fit very well into the API;
extracted MetadataCollection into interface, it could serve as one of few extension points for consumers (for possible caching).

Metadata:

\Doctrine\Performance\Annotations\DocLexerPerformanceBench

    benchMethod.............................I4 [μ Mo]/r: 7.058 7.076 (μs) [μSD μRSD]/r: 0.044μs 0.62%
    benchClass..............................I4 [μ Mo]/r: 10.018 10.060 (μs) [μSD μRSD]/r: 0.059μs 0.59%

\Doctrine\Performance\Annotations\DocParsePerformanceBench

    benchMethodParsing......................I4 [μ Mo]/r: 29.042 29.096 (μs) [μSD μRSD]/r: 0.119μs 0.41%
    benchClassParsing.......................I4 [μ Mo]/r: 28.667 28.728 (μs) [μSD μRSD]/r: 0.131μs 0.46%

\Doctrine\Performance\Annotations\PhpParserPerformanceWithoutShortCutBench

    bench...................................I4 [μ Mo]/r: 83.974 84.338 (μs) [μSD μRSD]/r: 0.527μs 0.63%

\Doctrine\Performance\Annotations\PhpParserPerformanceWithShortCutBench

    bench...................................I4 [μ Mo]/r: 88.739 88.268 (μs) [μSD μRSD]/r: 0.828μs 0.93%

\Doctrine\Performance\Annotations\ReadPerformanceBench

    bench...................................I4 [μ Mo]/r: 27.972 27.578 (μs) [μSD μRSD]/r: 0.822μs 2.94%

7 subjects, 35 iterations, 2,900 revs, 0 rejects, 0 failures, 0 warnings
(best [mean mode] worst) = 6.988 [39.353 39.306] 7.108 (μs)
⅀T: 1,377.349μs μSD/r 0.361μs μRSD/r: 0.940%

Master:

\Doctrine\Performance\Annotations\DocLexerPerformanceBench

    benchMethod.............................I4 [μ Mo]/r: 7.086 7.047 (μs) [μSD μRSD]/r: 0.063μs 0.89%
    benchClass..............................I4 [μ Mo]/r: 10.058 10.006 (μs) [μSD μRSD]/r: 0.106μs 1.05%

\Doctrine\Performance\Annotations\DocParsePerformanceBench

    benchMethodParsing......................I4 [μ Mo]/r: 22.700 22.633 (μs) [μSD μRSD]/r: 0.211μs 0.93%
    benchClassParsing.......................I4 [μ Mo]/r: 23.049 22.871 (μs) [μSD μRSD]/r: 0.368μs 1.59%

\Doctrine\Performance\Annotations\PhpParserPerformanceWithoutShortCutBench

    bench...................................I4 [μ Mo]/r: 84.378 84.647 (μs) [μSD μRSD]/r: 0.559μs 0.66%

\Doctrine\Performance\Annotations\PhpParserPerformanceWithShortCutBench

    bench...................................I4 [μ Mo]/r: 88.188 87.933 (μs) [μSD μRSD]/r: 0.656μs 0.74%

\Doctrine\Performance\Annotations\ReadPerformanceBench

    bench...................................I4 [μ Mo]/r: 23.430 23.196 (μs) [μSD μRSD]/r: 0.472μs 2.02%

7 subjects, 35 iterations, 2,900 revs, 0 rejects, 0 failures, 0 warnings
(best [mean mode] worst) = 7.016 [36.984 36.905] 7.180 (μs)
⅀T: 1,294.449μs μSD/r 0.348μs μRSD/r: 1.126%

Majkl578 · 2019-03-05T18:10:03Z

100% coverage in Metadata namespace. 😃

lib/Doctrine/Annotations/DocParser.php

lib/Doctrine/Annotations/Metadata/Exception/InvalidAnnotationTarget.php

alcaeus

Few comments below. A couple of general ideas:

Do you want to allow constructing exceptions without usage of the factories or should the constructor be private?
Since there are a lot of final classes, this becomes very dependent on our own classes without the option to properly customise anything. I think we should introduce interfaces for the builders (containing only the build method as everything else is an implementation detail) as well as an interface for the metadata classes (like you did with MetadataCollection). That way we're not closing down the entire API to the point where people will have to fork it if they want to change a single thing.

lib/Doctrine/Annotations/Metadata/Exception/InvalidAnnotationTarget.php

lib/Doctrine/Annotations/Metadata/Exception/MetadataAlreadyExists.php

lib/Doctrine/Annotations/Metadata/Exception/MetadataDoesNotExist.php

lib/Doctrine/Annotations/Metadata/Exception/TooManyDefaultProperties.php

Majkl578 · 2019-03-06T16:07:53Z

Do you want to allow constructing exceptions without usage of the factories or should the constructor be private?

Right now we allow it in Migrations and ORM master. Probably not a big deal, note that proper exception structure is yet to be introduced (converting AnnotationException to interface etc.).

Since there are a lot of final classes, this becomes very dependent on our own classes without the option to properly customise anything.

This is intentional. Most of the parts of Annotations should not be extended and those that could should be carefully considered.
I'd like to hear opinion from @Ocramius here, in my opinion the more we keep closed to modifications, the better, especially for such a widely used library.
Of course we could reconsider this later, before 2.0 or even after - unfinalizing a class should be BC-okay.

lib/Doctrine/Annotations/Metadata/Exception/InvalidAnnotationTarget.php

lib/Doctrine/Annotations/Metadata/AnnotationMetadata.php

lib/Doctrine/Annotations/Metadata/Builder/PropertyMetadataBuilder.php

lib/Doctrine/Annotations/Metadata/PropertyMetadata.php

Co-Authored-By: Majkl578 <[email protected]>

Majkl578 added Improvement RFC WIP labels Feb 23, 2019

Majkl578 requested review from Ocramius, lcobucci, guilhermeblanco and alcaeus February 23, 2019 09:13

Majkl578 force-pushed the metadata branch from 75cc1c1 to c6c11ed Compare February 23, 2019 22:41

Majkl578 marked this pull request as ready for review February 23, 2019 22:42

Majkl578 commented Feb 23, 2019

View reviewed changes

eugenekurasov reviewed Feb 26, 2019

View reviewed changes

lib/Doctrine/Annotations/DocParser.php Outdated Show resolved Hide resolved

Ocramius reviewed Feb 26, 2019

View reviewed changes

Ocramius added this to the v2.0.0 milestone Feb 26, 2019

Majkl578 force-pushed the metadata branch from c6c11ed to d7114b7 Compare February 26, 2019 18:05

Majkl578 requested a review from Ocramius February 26, 2019 18:31

alcaeus reviewed Feb 27, 2019

View reviewed changes

Majkl578 mentioned this pull request Feb 27, 2019

Upgrade Doctrine CS, use only for new files #253

Closed

Majkl578 force-pushed the metadata branch 3 times, most recently from d681cf4 to 4ff7b4b Compare March 5, 2019 14:54

Majkl578 requested a review from alcaeus March 5, 2019 15:03

Majkl578 force-pushed the metadata branch from 4ff7b4b to 4fe7430 Compare March 5, 2019 15:05

Majkl578 removed the WIP label Mar 5, 2019

Majkl578 force-pushed the metadata branch from 4fe7430 to b8eeb77 Compare March 5, 2019 18:06

Ocramius reviewed Mar 5, 2019

View reviewed changes

Majkl578 force-pushed the metadata branch from b8eeb77 to 257e93d Compare March 5, 2019 20:52

Majkl578 requested a review from Ocramius March 5, 2019 20:53

alcaeus reviewed Mar 6, 2019

View reviewed changes

Majkl578 force-pushed the metadata branch from e9c9d7e to 37e1dd8 Compare March 6, 2019 15:51

alcaeus previously approved these changes Mar 8, 2019

View reviewed changes

lib/Doctrine/Annotations/Metadata/Exception/InvalidAnnotationTarget.php Show resolved Hide resolved

Ocramius suggested changes Mar 13, 2019

View reviewed changes

Introduce annotation metadata

9cb3bff

Co-Authored-By: Majkl578 <[email protected]>

Majkl578 dismissed alcaeus’s stale review via 9cb3bff March 13, 2019 21:34

Majkl578 force-pushed the metadata branch from 37e1dd8 to 9cb3bff Compare March 13, 2019 21:34

Majkl578 requested review from Ocramius and alcaeus March 13, 2019 21:35

Ocramius approved these changes Mar 13, 2019

View reviewed changes

Ocramius self-assigned this Mar 13, 2019

Ocramius merged commit 734ddc7 into doctrine:master Mar 13, 2019

Majkl578 deleted the metadata branch March 13, 2019 21:51

Majkl578 mentioned this pull request Apr 3, 2019

[meta] Annotations 2.0 (ng) #232

Closed

27 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce annotation metadata #247

Introduce annotation metadata #247

Majkl578 commented Feb 23, 2019 •

edited

Loading

Majkl578 Feb 23, 2019

Majkl578 Feb 23, 2019

Ocramius left a comment

Ocramius commented Feb 26, 2019

Majkl578 commented Feb 26, 2019

Majkl578 commented Feb 26, 2019 •

edited

Loading

Ocramius commented Feb 26, 2019

alcaeus left a comment

alcaeus Feb 27, 2019

alcaeus Feb 27, 2019

Majkl578 Mar 5, 2019

alcaeus commented Mar 5, 2019

Majkl578 commented Mar 5, 2019

Majkl578 commented Mar 5, 2019

alcaeus left a comment

Majkl578 commented Mar 6, 2019 •

edited

Loading

Introduce annotation metadata #247

Introduce annotation metadata #247

Conversation

Majkl578 commented Feb 23, 2019 • edited Loading

Majkl578 Feb 23, 2019

Choose a reason for hiding this comment

Majkl578 Feb 23, 2019

Choose a reason for hiding this comment

Ocramius left a comment

Choose a reason for hiding this comment

Ocramius commented Feb 26, 2019

Majkl578 commented Feb 26, 2019

Majkl578 commented Feb 26, 2019 • edited Loading

Ocramius commented Feb 26, 2019

alcaeus left a comment

Choose a reason for hiding this comment

alcaeus Feb 27, 2019

Choose a reason for hiding this comment

alcaeus Feb 27, 2019

Choose a reason for hiding this comment

Majkl578 Mar 5, 2019

Choose a reason for hiding this comment

alcaeus commented Mar 5, 2019

Majkl578 commented Mar 5, 2019

Majkl578 commented Mar 5, 2019

alcaeus left a comment

Choose a reason for hiding this comment

Majkl578 commented Mar 6, 2019 • edited Loading

Majkl578 commented Feb 23, 2019 •

edited

Loading

Majkl578 commented Feb 26, 2019 •

edited

Loading

Majkl578 commented Mar 6, 2019 •

edited

Loading