WIP: Rule to convert action to attributes #257

andypost · 2023-10-29T02:14:06Z

Description

Rule to convert Action annotation to Attribute https://www.drupal.org/node/3395575

To-do https://www.drupal.org/node/3229001

To Test

using with core and examples module
vendor/bin/rector --xdebug -c rector.php -n process modules_/examples/modules/action_example

Drupal.org issue

https://www.drupal.org/project/rector/issues/3265946

andypost · 2023-10-29T02:14:44Z

1 file with changes
===================

1) modules_/examples/modules/action_example/src/Plugin/Action/BasicExample.php:1

    ---------- begin diff ----------
@@ @@

 namespace Drupal\action_example\Plugin\Action;

+use Drupal\Core\Action\Attribute\Action;
+use Drupal\Core\StringTranslation\TranslatableMarkup;
 use Drupal\Core\Access\AccessResult;
 use Drupal\Core\Action\ActionBase;
 use Drupal\Core\Messenger\MessengerInterface;
@@ @@

 /**
  * A basic example action that does nothing.
- *
- * @Action(
- *   id = "action_example_basic_action",
- *   label = @Translation("Action Example: A basic example action that does nothing"),
- *   type = "system"
- * )
  */
+#[Action(id: 'action_example_basic_action', label: new TranslatableMarkup('Action Example: A basic example action that does nothing'), type: 'system')]
 class BasicExample extends ActionBase implements ContainerFactoryPluginInterface {

   /**
    ----------- end diff -----------

Applied rules:
 * ActionAnnotationToAttributeRector (https://docs.phpunit.de/en/10.0/annotations.html#ticket)

bbrala · 2023-10-29T08:50:33Z

Good start ❤️, thank you for working on this.

I'd probably make it a ConfigurableRector for one where you can feed it a configuration valueobject. I'm assuming the input for this rector would always be the tagname, since that should always map 1-on-1 to the attribute (class)? (see FunctionToStaticValueObject that implemets VersionedConfigurationInterface) for an example of such an implementation.

Then it could work like a backwards compatible rule, where we pass the version it was introduced in (as FunctionToStaticRector). But it also means it needs new handling in the AbstractDrupalCoreRector since the BC call is not really a call (which can be called using DeprecationHelper), but the fact it does not remove the docblock annotatation. Thinking about that, it might need some refactoring to handle this case of it being a docblock, or we need to handle that in the rector itself in this case. But i need to be more awake to think about that one.

Random thought, what if we make the removal bases on a property of ActionAnnotationToAttributeRector which we can set with a setter? Then we can toggle it easily. By default remove the docblock, but allow $rector->setDocblockRemoval(false) to disable it? This could be an extra argument in the ValueObject also maybe that forces the removal/keep behaviour?

The end result would be:

A ConfigurableRector that can handle refactoring any of the coming annotation changes. As long as we are consistent in the change we just add a new rule with the correct configuration
A way to provide the change with or without keeping the original docblock (BC fixes yay).
Easy way to convert core (which can be configurable locallly to force removal even if you are running it against 10.x)
Profit.

When looking at the core change record i did also notice the following:

/*
* @Block(
 *   id = "test_block_instantiation",
 *   admin_label = @Translation("Display message")
 * )
 */
#[Block(
  id: "test_block_instantiation",
  admin_label: new TranslatableMarkup("Display message")
)]

This does mean there is nested tags going on? Do we know what those are? Is that a fixed set?

andypost · 2023-10-29T14:59:42Z

There's issue about formatting as miltiline rectorphp/rector#8239

andypost · 2023-10-29T15:05:18Z

The set keys of annotation is defined by class https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Core/Block/Annotation/Block.php

Technically the annotation could be parsed, child nodes could be picked from properties' doc-blocks and mapped

@var \Drupal\Core\Annotation\Translation
@var \Drupal\Core\Annotation\ContextDefinition[]

andypost · 2023-10-29T15:09:35Z

src/Rector/Deprecation/ActionAnnotationToAttributeRector.php

+        $args = [];
+        foreach ($parsedArgs as $value) {
+            if ($value->key === 'label') {
+                $arg = new Node\Expr\New_(new Node\Name(TranslatableMarkup::class), [new Arg(new String_($value->value->values[0]->value->value))]);


it could be a helper method

Berdir · 2023-10-30T20:59:22Z

There's issue about formatting as miltiline rectorphp/rector#8239

I don't think that's related. That's not about generating multiline attributes, it's about parsing annotations with multiline strings.

andypost · 2023-10-30T21:20:26Z

To apply coding standards, according to https://github.com/rectorphp/rector/blob/main/README.md#how-to-apply-coding-standards

https://github.com/rectorphp/rector-src/blob/main/ecs.php

mstrelan · 2023-12-05T03:57:12Z

src/Rector/Deprecation/ActionAnnotationToAttributeRector.php

+                $arg = new Node\Expr\New_(new Node\Name(TranslatableMarkup::class), [new Arg(new String_($value->value->values[0]->value->value))]);
+            }
+            else {
+                $arg = new String_($value->value->value);


In our internal projects we tend to use a const PLUGIN_ID = 'foo' pattern and reference these in our annotations. I've been playing around with a custom annotation to attribute rector rule based on this one and noticed it fails on these constants. The following seems to work, although I'm wondering if there is a better way to detect the use of a constant in the annotation.

$arg = new String_($value->value instanceof StringNode ? new String_($value->value->value) : new ConstFetch(new Node\Name($value->value)));

See root comment.

bbrala · 2023-12-12T12:26:00Z

What changed yesterday (AI generated):

In the ActionAnnotationToAttributeRector.php file, the convertTranslateAnnotation method had its input parameter type and internal logic modified to work directly with the actual annotation instead of its container, simplifying the conversion process.
Another significant modification is made in handling method annotation-to-attribute conversions. To support backward compatibility in refactors, the changes allow annotative refactorings only on Drupal version 10.1 or higher, ignoring lower versions.
AbstractDrupalCoreRector.php was refactored to improve the readability of the methods, ensuring return types match the method names. It also checks if the Drupal version supports backward compatibility.
The Drupal version in Drupal.php (\Drupal::VERSION) was updated to '10.2.x-dev'.

These changes seem to streamline the annotation handling in Drupal and improve compatibility with earlier versions.

Note:
I do think at some points we'd want a service to get the Drupal version. Then we can actually set the version we run against in the tests if we like, which will make a few things easier.

andypost · 2024-02-29T06:55:02Z

New conversions

ImageToolkitOperation https://git.drupalcode.org/project/drupal/-/commit/328bb474d7
ImageToolkit https://git.drupalcode.org/project/drupal/-/commit/ffaf9871607dbfe42c976da2109b0d04f15faada
ImageEffect https://git.drupalcode.org/project/drupal/-/commit/58ecf6d433

andypost · 2024-03-04T09:52:43Z

Added the rest of conversion https://www.drupal.org/node/3229001

…guments.

…return types match the method names).

… ActionAnnotationToAttributeRector to use AbstractDrupalCoreRector (mostly)

…y taking the actual annotation instead of its container.

…e configuration to more targetted.

…notation

config/drupal-10/drupal-10.2-deprecations.php

src/Drupal10/Rector/Deprecation/ActionAnnotationToAttributeRector.php

mglaman · 2024-03-08T15:06:49Z

src/Drupal10/Rector/Deprecation/AnnotationToAttributeRector.php

+            if ($hasAttribute === false) {
+                $stringValue = $valueNode->value->value;
+                $stringValue = '{'.trim($stringValue, '()').'}';
+                $tokenIterator = $this->tokenIteratorFactory->create($stringValue);
+                $data = $this->arrayParser->parseCurlyArray($tokenIterator, $node);
+                $attribute = $this->createAttribute($configuration->getAttributeClass(), $data);
+                $node->attrGroups[] = new AttributeGroup([$attribute]);
+            }


Can this do a better check that the annotation matching the ending class name?

Wouldn't this cause @foo to become #Foo attribute because its a generic string?

Do you mean here; you get to this code after checking a tag is present as configured. Then we move on to check if there is already an attribute. If that is present, then we loopt through the tagnames and if there was no attribute matching the relevant attribute class from the configuration we start building the attribute based on the class passed into the rector by the configuration.

Don't really see how you could break this, unless user error using their own config?

More so just:

$blah = explode('\', $configuration->getAttributeClass()); $className = end($blah); if ($blah === substr($stringValue, 1)) {

Not entirely configured. Just that the tag matches the attribute's class name. I'm going to bet there is a "malformed" phpdoc somewhere with what the phpdoc parser considers generic tags

…ter validation on these.

bbrala · 2024-03-08T15:27:13Z

Perhaps we could do more with this one by rector itself:

AnnotationToAttributeMapper $annotationToAttributeMapper
$attribute = $this->annotationToAttributeMapper->map($value);

This class (which allowd extending classes by interface) does smart things with values. It also does stuff like mapping consts and such.

Kinda worth exploring if we can maybe just hook into that for converting some. The code looked very interesting to structure the mapping. Those classes were made for doctrine itself.

bbrala · 2024-03-08T15:32:42Z

New conversions

ImageToolkitOperation https://git.drupalcode.org/project/drupal/-/commit/328bb474d7

ImageToolkit https://git.drupalcode.org/project/drupal/-/commit/ffaf9871607dbfe42c976da2109b0d04f15faada

ImageEffect https://git.drupalcode.org/project/drupal/-/commit/58ecf6d433

BTW want to mention, these three would've been automatable with this MR without any problem. All known properties.

mglaman

Approving. The rule isn't configured and this allows using it for Drupal core improvements.

andypost force-pushed the action-attributes branch 2 times, most recently from 714f6bb to 5f4a4e8 Compare October 29, 2023 02:51

andypost commented Oct 29, 2023

View reviewed changes

agentrickard added the work-in-progress label Oct 31, 2023

mstrelan reviewed Dec 5, 2023

View reviewed changes

bbrala mentioned this pull request Dec 10, 2023

[Ignore] Running tests for attributes while waiting for merge #276

Closed

andypost force-pushed the action-attributes branch 2 times, most recently from 676af5b to dcffdd9 Compare December 10, 2023 17:55

bbrala marked this pull request as ready for review December 11, 2023 17:41

bbrala added needs-review and removed work-in-progress labels Dec 11, 2023

bbrala requested a review from mglaman December 11, 2023 17:42

bbrala force-pushed the action-attributes branch from 5b5bc68 to 401e127 Compare January 14, 2024 15:21

andypost and others added 8 commits March 8, 2024 09:57

WIP: Rule to convert action to attributes

1b8c349

Rebase on main and move rector to Drupal10 namespace

4ec1177

Add basic unit test for ActionAnnotationToAttributeRector.

ecf029a

Fix codestyle

407fb52

Fix namespace in config for ActionAnnotationToAttributeRector

d936f39

The label should be "action_label",

bbe8c03

Fix phpstan

b383c04

Add test for other possible translation attribute arguments

c0fbf2e

bbrala added 10 commits March 8, 2024 09:57

Add test for situation where the attribute already exists

47c1bcc

If action attribute is already present, do not add it again.

0947c65

ActionAnnotationToAttributeRector now also parses extra @translate ar…

75a061e

…guments.

Fix code-style

283d96b

Refactor some methods in AbstractDrupalCoreRector

98faa8d

Fix some logic in AbstractDrupalCoreRector so its more readable (and …

331b631

…return types match the method names).

Introduce DrupalIntroducedAndRemovalVersionConfiguration and refactor…

9922589

… ActionAnnotationToAttributeRector to use AbstractDrupalCoreRector (mostly)

Add tests for BC path and update default tests to expect no BC path

6dad267

Fix convertTranslateAnnotation early return. Also simplify a little b…

651332f

…y taking the actual annotation instead of its container.

Fix style

773255a

bbrala force-pushed the action-attributes branch from 401e127 to 773255a Compare March 8, 2024 08:57

bbrala added 7 commits March 8, 2024 10:01

Remove prefixed class from use statements.

f249f01

Allow rule to be configured so we can use it on multiple rules. Renam…

262dffa

…e configuration to more targetted.

Rename fixtures to add plugin id

8f0feda

Add block test and configuration

58566af

Add missing deriver key

ce8ad53

Use built in AnnotationToAttribute mapper for forms value in Block an…

fc50d58

…notation

Fix codestyle

0d26dd5

mglaman reviewed Mar 8, 2024

View reviewed changes

bbrala added 2 commits March 8, 2024 16:12

Fully remove annotations from drupal 10 set for now until we have bet…

85f4742

…ter validation on these.

Protected to public

12ebdc7

mglaman approved these changes Mar 8, 2024

View reviewed changes

agentrickard added ready-to-merge and removed needs-review labels Mar 8, 2024

agentrickard merged commit 7a48c24 into palantirnet:main Mar 8, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Rule to convert action to attributes #257

WIP: Rule to convert action to attributes #257

andypost commented Oct 29, 2023 •

edited

Loading

andypost commented Oct 29, 2023 •

edited

Loading

bbrala commented Oct 29, 2023 •

edited

Loading

andypost commented Oct 29, 2023

andypost commented Oct 29, 2023

andypost Oct 29, 2023

Berdir commented Oct 30, 2023

andypost commented Oct 30, 2023

mstrelan Dec 5, 2023

bbrala Mar 8, 2024

bbrala commented Dec 12, 2023 •

edited

Loading

andypost commented Feb 29, 2024

andypost commented Mar 4, 2024

mglaman Mar 8, 2024

bbrala Mar 8, 2024

mglaman Mar 8, 2024

bbrala commented Mar 8, 2024

bbrala commented Mar 8, 2024

mglaman left a comment

WIP: Rule to convert action to attributes #257

WIP: Rule to convert action to attributes #257

Conversation

andypost commented Oct 29, 2023 • edited Loading

Description

To Test

Drupal.org issue

andypost commented Oct 29, 2023 • edited Loading

bbrala commented Oct 29, 2023 • edited Loading

andypost commented Oct 29, 2023

andypost commented Oct 29, 2023

andypost Oct 29, 2023

Choose a reason for hiding this comment

Berdir commented Oct 30, 2023

andypost commented Oct 30, 2023

mstrelan Dec 5, 2023

Choose a reason for hiding this comment

bbrala Mar 8, 2024

Choose a reason for hiding this comment

bbrala commented Dec 12, 2023 • edited Loading

andypost commented Feb 29, 2024

andypost commented Mar 4, 2024

mglaman Mar 8, 2024

Choose a reason for hiding this comment

bbrala Mar 8, 2024

Choose a reason for hiding this comment

mglaman Mar 8, 2024

Choose a reason for hiding this comment

bbrala commented Mar 8, 2024

bbrala commented Mar 8, 2024

mglaman left a comment

Choose a reason for hiding this comment

andypost commented Oct 29, 2023 •

edited

Loading

andypost commented Oct 29, 2023 •

edited

Loading

bbrala commented Oct 29, 2023 •

edited

Loading

bbrala commented Dec 12, 2023 •

edited

Loading