- 🇮🇹Italy gambry Milan
Swapping the Drupal Global Contribution Weekend 2023 tag with the correct one.
- First commit to issue fork.
- 🇮🇳India bhanu951
Add missing return statement and fix CCF.
Rebased to latest head.# 132 📌 Create a trait and base class to implement \Drupal\Component\Plugin\ConfigurableInterface Needs review and # 123 📌 Create a trait and base class to implement \Drupal\Component\Plugin\ConfigurableInterface Needs review still need discussion.
- 🇺🇸United States mikelutz Michigan, USA
mikelutz → changed the visibility of the branch 11.x to hidden.
- 🇺🇸United States mikelutz Michigan, USA
mikelutz → changed the visibility of the branch 11.x to hidden.
- 🇺🇸United States mikelutz Michigan, USA
mikelutz → changed the visibility of the branch 2852463-configurable-plugin-trait-10.1 to hidden.
- 🇺🇸United States mikelutz Michigan, USA
mikelutz → changed the visibility of the branch 9.2.x to hidden.
- 🇺🇸United States mikelutz Michigan, USA
mikelutz → changed the visibility of the branch 2852463-create-a-trait to hidden.
- Status changed to Needs review
6 months ago 4:27pm 8 July 2024 - 🇺🇸United States mikelutz Michigan, USA
Alright, I think I've resurrected this from the ashes. New merge request on 11.x, fixed issues in #123and #132
123.1 Mention the interface instead of the trait in ConfigurablePluginBase.
I mention both, since the base class uses the trait anyway.
123.2 In ConfigurableTrait, document that the $configuration property is also defined \Drupal\Component\Plugin\PluginBase.
done
123.3 Decide on if ConfigurableTraitTest needs adjustments.
It did, though opposite from the previous suggestions of moving all tests to getMockForTrait, as getMockForTrait is getting deprecated in phpunit, so I converted all tests to use the same concrete test class that uses the trait.
123.4 Make sure that the changes in base classes for certain plugin types are mentioned in the release notes.
I added the list to the release notes snippet and the CR
123.5 (optional) Check if setConfiguration() still needs to be overridden in WorkflowTypeBase.
It's an issue of overriding it and keeping tests, or resetting it to the standard and adjusting all the tests to handle the reordered returned array. I also removed use of the trait entirely from ConditionPluginBase for the same reasons, updating it to use mergeDeepArray broke a few tests and will require adjusting the order of items in umami config to pass tests (as they would be exported in a different order, causing a test fail) It's better to explore converting both of those pluginbases to use mergeDeepArray in a followup, if at all, where we can review the test changes and potential BC issues more thoroughly in isolation from the trait and base class work being provided here.
123.6 Rebase the merge request.
done.
132 > implement \Drupal\Component\Plugin\ConfigurableTrait directly
Is that the right wording? Does one 'implement a trait'? I'd have said 'use' or 'import'.
I agree, switched the wording to use `use`.
In addition, I added SelectionPluginBase to the plugin bases that we convert (it was added after the original MR was created here), and removed ConditionPluginBase for reasons described above.
I moved ConfigurableTrait from Component/Plugin to Core/Plugin. In the original patch, we didn't notice that having the trait in Component meant we added a undocumented dependency on Component/Utility to Component/Plugin, and we shouldn't do that. The interface can live in the Component, the trait can live in Core next to the base class (which was already there because that needed to extend Core's PluginBase and not Component's. Despite the fact that the interface lives in Component, it makes more sense to have the implementations live in Core.
I updated the Change Record (and unpublished it, as it was never unpublished after we reverted years ago.
I fixed a few coding standards/phpstan/phpunit10 issues that have changed things over the years.
I think this should be committable now, or close to it hopefully.
- First commit to issue fork.
- Status changed to RTBC
4 months ago 1:20pm 22 August 2024 - 🇺🇸United States smustgrave
Opened 📌 Determine if we can remove logger from ImageEffectBase Active to research if we can remove logger.
All I did was rebase (200+ commits back) and added some typehints to the test classes.
Reviewed the rest of the changes and looks cleaner. Didn't see anything off so lets see if we can land this old one.
- 🇳🇿New Zealand quietone
I read the IS, the comments and the MR. There are suggestions in the MR for the comments and they do need someone working on this issue to review, so back to NW for that.
And a follow up is needed for this part of #145
It's better to explore converting both of those pluginbases to use mergeDeepArray in a followup, if at all, where we can review the test changes and potential BC issues more thoroughly in isolation from the trait and base class work being provided here.