feat: Add support to php 8 attributes #2241

djpremier · 2021-07-16T00:25:05Z

djpremier · 2021-07-16T00:26:34Z

Helps are very much welcome 🙂

ker0x · 2021-08-03T12:59:07Z

src/Mapping/Annotation/Blameable.php

+     *
+     * @return void
+     */
+    public function __construct($on = 'update', $field = null, $value = null)


Suggested change

public function __construct($on = 'update', $field = null, $value = null)

public function __construct(string $on = 'update', $field = null, $value = null)

The other fields should probably be typed as well then? ?string

Unfortunatly no, according to the PHPDoc, they can have multiple types, and union types is only available in PHP 8

/** @var string|array */ public $field; /** @var mixed */ public $value;

ker0x · 2021-08-03T12:59:28Z

src/Mapping/Annotation/IpTraceable.php

+     *
+     * @return void
+     */
+    public function __construct($on = 'update', $field = null, $value = null)


Suggested change

public function __construct($on = 'update', $field = null, $value = null)

public function __construct(string $on = 'update', $field = null, $value = null)

ker0x · 2021-08-03T12:59:54Z

src/Mapping/Annotation/Loggable.php

+     *
+     * @return void
+     */
+    public function __construct($logEntryClass = null)


Suggested change

public function __construct($logEntryClass = null)

public function __construct(?string $logEntryClass = null)

ker0x · 2021-08-03T13:02:20Z

src/Mapping/Annotation/Slug.php

+        $fields = [],
+        $updatable = true,
+        $style = 'default',
+        $unique = true,
+        $unique_base = null,
+        $separator = '-',
+        $prefix = '',
+        $suffix = '',
+        $handlers = [],
+        $dateFormat = 'Y-m-d-H:i'


Suggested change

$fields = [],

$updatable = true,

$style = 'default',

$unique = true,

$unique_base = null,

$separator = '-',

$prefix = '',

$suffix = '',

$handlers = [],

$dateFormat = 'Y-m-d-H:i'

array $fields = [],

bool $updatable = true,

string $style = 'default',

bool $unique = true,

?string $unique_base = null,

string $separator = '-',

string $prefix = '',

string $suffix = '',

array $handlers = [],

string $dateFormat = 'Y-m-d-H:i'

ker0x · 2021-08-03T13:02:53Z

src/Mapping/Annotation/SoftDeleteable.php

+     *
+     * @return void
+     */
+    public function __construct($fieldName = 'deletedAt', $timeAware = false, $hardDelete = true)


Suggested change

public function __construct($fieldName = 'deletedAt', $timeAware = false, $hardDelete = true)

public function __construct(string $fieldName = 'deletedAt', bool $timeAware = false, bool $hardDelete = true)

ker0x · 2021-08-03T13:05:43Z

src/ReferenceIntegrity/Mapping/Driver/Attribute.php

+    /**
+     * Annotation to identify the fields which manages the reference integrity
+     */
+    const REFERENCE_INTEGRITY = 'Gedmo\\Mapping\\Annotation\\ReferenceIntegrity';


Suggested change

const REFERENCE_INTEGRITY = 'Gedmo\\Mapping\\Annotation\\ReferenceIntegrity';

const REFERENCE_INTEGRITY = ReferenceIntegrity::class;

ker0x · 2021-08-03T13:06:09Z

src/ReferenceIntegrity/Mapping/Driver/Attribute.php

+    /**
+     * ReferenceIntegrityAction extension annotation
+     */
+    const ACTION = 'Gedmo\\Mapping\\Annotation\\ReferenceIntegrityAction';


Suggested change

const ACTION = 'Gedmo\\Mapping\\Annotation\\ReferenceIntegrityAction';

const ACTION = ReferenceIntegrityAction::class;

ndench

Thanks for you contribution! I think all of @ker0x suggestions are valid, we might as well type the properties since the min supported version is 7.2.

ndench · 2021-08-05T04:18:18Z

src/Mapping/Annotation/Blameable.php

+     *
+     * @return void
+     */
+    public function __construct($on = 'update', $field = null, $value = null)


The other fields should probably be typed as well then? ?string

ndench · 2021-08-05T04:22:51Z

src/ReferenceIntegrity/Mapping/Driver/Attribute.php

+    /**
+     * {@inheritdoc}
+     */
+    public function readExtendedMetadata($meta, array &$config)


This appears to be copy pasted from \Gedmo\ReferenceIntegrity\Mapping\Driver\Annotation. Should it maybe extend the Annotation driver and overwrite the AnnotationReader? Or perhaps decorate it?

AkenRoberts · 2021-09-09T02:05:51Z

src/Mapping/Annotation/Blameable.php

+     *
+     * @return void
+     */
+    public function __construct($on = 'update', $field = null, $value = null)


This is a breaking change (removing the Annotation parent and changing the constructor signature). It would require a major release. Is it necessary, or can we find a backwards-compatible option?

It's technically a break, but in terms of usage it should be fine because of the @NamedArgumentConstructor annotation: https://www.doctrine-project.org/projects/doctrine-annotations/en/1.13/custom.html#optional-constructors-with-named-parameters

So there is no need of extending Annotation class anymore.

It's a bit of a weird case. From a purely PHP API perspective, it is 100% a B/C break:

Class inheritance changes ($foo instanceof Doctrine\Common\Annotations\Annotation goes from passing to failing with this PR applied)

Constructor signature changes

From a practical perspective, it really starts to get murky. Gedmo\Mapping\Driver\AnnotationDriverInterface doesn't have a hard dependency on the Doctrine\Common\Annotations\Reader interface, so there isn't an assurance that the doctrine/annotations package is even being used to read the annotations. Even if there were, if someone is implementing the interface themselves and not using the readers or parser from that package (Doctrine\Common\Annotations\DocParser is where the logic to deal with the @NamedArgumentConstructor annotation lies), that reader might not be able to construct the annotation classes with the changed constructor signatures. Granted, the odds of someone using a custom annotation reader and NOT using their parser are pretty low, but even if Doctrine's interfaces were typehinted, you can't guarantee a reader is using that compatibility layer.

Thanks for the explanation, so what we can do is to keep the array $data = [] as first parameter of the constructor and add the rest after it.

I've re-added it in #2253

franmomu · 2021-08-13T05:19:31Z

src/Mapping/Annotation/Blameable.php

+     *
+     * @return void


Suggested change

*

* @return void

In all constructors

franmomu · 2021-09-23T21:10:17Z

src/Mapping/Annotation/Blameable.php

+     *
+     * @return void
+     */
+    public function __construct($on = 'update', $field = null, $value = null)


It's technically a break, but in terms of usage it should be fine because of the @NamedArgumentConstructor annotation: https://www.doctrine-project.org/projects/doctrine-annotations/en/1.13/custom.html#optional-constructors-with-named-parameters

So there is no need of extending Annotation class anymore.

art-cg · 2021-12-06T10:06:47Z

Hi. @djpremier Thanks for starting the PR. Are there any news of the actual state?
I saw that 3.4.0 includes the first PHP8-Attributes
https://github.com/doctrine-extensions/DoctrineExtensions/releases/tag/v3.4.0

franmomu · 2021-12-06T11:15:34Z

Hi. @djpremier Thanks for starting the PR. Are there any news of the actual state? I saw that 3.4.0 includes the first PHP8-Attributes https://github.com/doctrine-extensions/DoctrineExtensions/releases/tag/v3.4.0

I've created #2363 to keep track of the needed work.

franmomu · 2021-12-19T09:52:41Z

Closing in favor of #2363, thanks for starting this @djpremier!

feat: Add support to php 8 attributes

Verified

This commit was signed with the committer’s verified signature.

djpremier Diefferson Koderer Môro

GPG key ID: 432367BE02E973F8

Verified
Learn about vigilant mode

5299b5e

ker0x reviewed Aug 3, 2021

View reviewed changes

ndench reviewed Aug 5, 2021

View reviewed changes

AkenRoberts reviewed Sep 9, 2021

View reviewed changes

franmomu reviewed Sep 23, 2021

View reviewed changes

phansys added the New Feature label Nov 5, 2021

phansys added Needs Rebase Needs Changelog note labels Dec 6, 2021

franmomu closed this Dec 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Sponsors

feat: Add support to php 8 attributes #2241

feat: Add support to php 8 attributes #2241

djpremier commented Jul 16, 2021 •

edited

Loading

djpremier commented Jul 16, 2021

ker0x Aug 3, 2021

ndench Aug 5, 2021

ker0x Aug 5, 2021

ker0x Aug 3, 2021

ker0x Aug 3, 2021

ker0x Aug 3, 2021

ker0x Aug 3, 2021

ker0x Aug 3, 2021

ker0x Aug 3, 2021

ndench left a comment

ndench Aug 5, 2021

ndench Aug 5, 2021

AkenRoberts Sep 9, 2021

franmomu Sep 23, 2021

mbabker Oct 5, 2021

franmomu Oct 5, 2021

franmomu Aug 13, 2021

franmomu Sep 23, 2021

art-cg commented Dec 6, 2021

franmomu commented Dec 6, 2021

franmomu commented Dec 19, 2021 •

edited

Loading

	public function __construct($on = 'update', $field = null, $value = null)
	public function __construct(string $on = 'update', $field = null, $value = null)

	public function __construct($logEntryClass = null)
	public function __construct(?string $logEntryClass = null)

	public function __construct($fieldName = 'deletedAt', $timeAware = false, $hardDelete = true)
	public function __construct(string $fieldName = 'deletedAt', bool $timeAware = false, bool $hardDelete = true)

	const REFERENCE_INTEGRITY = 'Gedmo\\Mapping\\Annotation\\ReferenceIntegrity';
	const REFERENCE_INTEGRITY = ReferenceIntegrity::class;

	const ACTION = 'Gedmo\\Mapping\\Annotation\\ReferenceIntegrityAction';
	const ACTION = ReferenceIntegrityAction::class;

feat: Add support to php 8 attributes #2241

feat: Add support to php 8 attributes #2241

Conversation

djpremier commented Jul 16, 2021 • edited Loading

djpremier commented Jul 16, 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

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

ndench 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

art-cg commented Dec 6, 2021

franmomu commented Dec 6, 2021

franmomu commented Dec 19, 2021 • edited Loading

djpremier commented Jul 16, 2021 •

edited

Loading

franmomu commented Dec 19, 2021 •

edited

Loading