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
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:
- 3484085-attributes-support
changes, plain diff MR !58
- attributes
changes, plain diff MR !38
Comments
Comment #3
ptmkenny commentedComment #4
megachrizAlright, 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?
Comment #5
ptmkenny commentedThanks 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.
Comment #6
ptmkenny commentedTests in #3484089: Convert annotations to attributes in Tamper plugins (requires 10.2+) are failing; those should be fixed before this is committed.
Comment #7
megachrizI'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.
Comment #8
ptmkenny commentedAs 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.
Comment #9
ptmkenny commentedHmm, 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?
Comment #11
ptmkenny commentedClosed in favor of #3484089: Convert annotations to attributes in Tamper plugins (requires 10.2+)
Comment #13
megachrizI 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.
Comment #16
ptmkenny commentedOk, 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.
Comment #17
megachrizI 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...
Comment #18
megachrizBecause \Drupal\tamper\Attribute\Tamper extends \Drupal\Component\Plugin\Attribute\Plugin. The latter class is not available in Drupal versions before 10.2.
Comment #19
megachrizI 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.