Problem/Motivation

Drupal core is switching from Annotations to Attributes. Tamper should also provide an Attribute class so that downstream modules can start converting.

This MR will just add an Attribute class without actually adding attributes to the tamper plugins because the module still supports Drupal 8 and 9, but PHP attribute support is only available with PHP 8.1.

So, I propose adding attribute support now and then adding a follow-up issue that actually converts all the plugins to use attributes as well as annotations (which should be postponed until support for Drupal versions before 10.2 is dropped).

Issue fork tamper-3484085

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

ptmkenny created an issue. See original summary.

ptmkenny’s picture

Issue summary: View changes
megachriz’s picture

Alright, good idea. I think that we can only test this by writing a custom Tamper plugin? Maybe it would be good to have a Tamper plugin using attributes in a test module, so that we can add test coverage for this addition?

ptmkenny’s picture

Status: Active » Needs review

Thanks for the super quick response. As for testing this, I just converted all the annotations to attributes in #3484089: Convert annotations to attributes in Tamper plugins (requires 10.2+). That can't be committed until this module requires Drupal > 10.2, but it does give test coverage of the attribute class (since all the tests will be run there with attributes instead of annotations), but then of course the test is in a separate issue instead of in this one, where the class will initially be committed.

ptmkenny’s picture

Status: Needs review » Needs work

Tests in #3484089: Convert annotations to attributes in Tamper plugins (requires 10.2+) are failing; those should be fixed before this is committed.

megachriz’s picture

I'm not sure if I want to require Drupal 10.2 yet, so because of that it might be useful to add test coverage for this.

ptmkenny’s picture

As you referenced in the other issue, TamperManager needs to be updated to match this change record: https://www.drupal.org/node/3395582

I'll try to get to this tomorrow. I'll fix this issue and the other one first, and then see about adding a test just for this one.

ptmkenny’s picture

Status: Needs work » Needs review

Hmm, adding this change breaks Drupal 9, so perhaps this issue should be closed in favor of #3484089: Convert annotations to attributes in Tamper plugins (requires 10.2+), which then that issue can be committed someday when support for older versions of Drupal are dropped?

ptmkenny’s picture

Status: Needs review » Closed (duplicate)

megachriz’s picture

Status: Closed (duplicate) » Needs review

I think it is nice if we can add support for attributes now, so I'm reopening this. I do think of dropping Drupal 9 support after the next release.
Also because for Drupal 12 it will be required for plugin types to support attributes: https://www.drupal.org/node/3522776.

megachriz changed the visibility of the branch attributes to hidden.

ptmkenny’s picture

Ok, cool. Should we add the tamper attributes to the existing plugins (without removing the annotations)? I'm pretty sure the only thing this requires is PHP 8.1.

megachriz’s picture

I think that adding Attribute does require Drupal 10.2? Anyway, the Attribute code could be updated in #3484089: Convert annotations to attributes in Tamper plugins (requires 10.2+), though there is a good chance they would need to be updated again when #3541650: Make Tamper plugins tell if they require a tamperable item gets merged.

@ptmkenny
If you like and have the time, there are a bunch of Tamper issues updated that can be reviewed/tested. :)
I keep track of issues that I work on in a spreadsheet: https://docs.google.com/spreadsheets/d/1cn4vmHosTfxgyI0zlvVv_ce94715eH-m...

megachriz’s picture

I think that adding Attribute does require Drupal 10.2?

Because \Drupal\tamper\Attribute\Tamper extends \Drupal\Component\Plugin\Attribute\Plugin. The latter class is not available in Drupal versions before 10.2.

megachriz’s picture

Status: Needs review » Fixed

I checked and tested the code again. The test Tamper plugin that is defined using Attribute, is visible in the Feeds UI, the ECA UI and the Migrate Plus Feeds UI. So this works good. Merged.

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

  • megachriz committed 4b9566ee on 8.x-1.x
    Issue #3484085 by ptmkenny, megachriz: Added support for Attributes (...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.