FillPdfActionPluginInterface is extending the deprecated ConfigurablePluginInterface, even though our FillPdfActionPluginBase only implements stub methods that aren't used by any of our three FillPdfActionPlugins and it is unclear whether we would ever be using it.

While a replacement is provided from Drupal 8.7, we're still supporting Drupal 8.6.

To avoid deprecation notices, we may remove ConfigurablePluginInterface. While we could also remove its stub implementations, there would be a minor risk some user may have implemented a custom FillPdfActionPlugin relying on ConfigurablePluginInterface's methods. While it is imaginable that some custom code checks instanceof ConfigurablePluginInterface, this seems a bit far-fetched.

I'd therefore propose replacing ConfigurablePluginInterface by DependentPluginInterface in FillPdfActionPluginInterface, while deprecating its stub implementations in FillPdfActionPluginBase.

CommentFileSizeAuthor
#11 3058862_9-11_interdiff.txt4.08 KBPancho
#11 fillpdf_action_plugins_3058862_11.patch5.07 KBPancho
#10 3058862_3-9_interdiff.txt4.09 KBPancho
#10 fillpdf_action_plugins_3058862_9.patch5.08 KBPancho
#9 3058957_3-5_interdiff.txt930 bytesPancho
#9 3058957-5.patch8.85 KBPancho
#3 3058862_2-3_interdiff.txt1.55 KBPancho
#3 fillpdf_action_plugins_3058862_3.patch4.19 KBPancho
#2 fillpdf_action_plugins_3058862_2.patch3.31 KBPancho
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Pancho created an issue. See original summary.

Pancho’s picture

Pancho
Location UTC+2 🇪🇺 EU
CreditAttribution: Pancho as a volunteer commented
Assigned: Unassigned » wizonesolutions
Issue summary: View changes
Status: Active » Needs review
FileSize
fillpdf_action_plugins_3058862_2.patch3.31 KB
Pancho’s picture

Pancho
Location UTC+2 🇪🇺 EU
CreditAttribution: Pancho as a volunteer commented
FileSize
fillpdf_action_plugins_3058862_3.patch4.19 KB
3058862_2-3_interdiff.txt1.55 KB

We also need to trigger an error.

wizonesolutions’s picture

wizonesolutions
he/him
Primary language English
Location Norway
CreditAttribution: wizonesolutions as a volunteer and at WizOne Solutions for FillPDF Service commented

Hmm, base classes and interfaces are supposed to provide BC. Would be good if we could do that here.

Pancho’s picture

Pancho
Location UTC+2 🇪🇺 EU
CreditAttribution: Pancho as a volunteer commented

True, but please tell me where I missed any BC...

I'm replacing the deprecated ConfigurablePluginInterface by DependentPluginInterface and only am deprecating all other implementations of ConfigurablePluginInterface. So while they are no longer present on the interface, any custom classes extending FillPdfActionPluginBase shuouldn't break at all.

wizonesolutions’s picture

wizonesolutions
he/him
Primary language English
Location Norway
CreditAttribution: wizonesolutions as a volunteer and at WizOne Solutions for FillPDF Service commented

In looking at this again, I think I agree with you now. Any instanceof type-hinting should be against FillPdfActionPluginInterface—at least, that's all we should care about.

I'm not sure if we're even implementing either of those interfaces, to be honest? We aren't calculating any dependencies, are we? But I guess we might later on for certain plugin types? (if we use actions for arbitrary operations on a PDF later or something)

Pancho’s picture

Pancho
Location UTC+2 🇪🇺 EU
CreditAttribution: Pancho as a volunteer commented

I'm not sure if we're even implementing either of those interfaces, to be honest

No, we're not and I don't know if we would do so in the future.

However, our users may rely on FillPdfActionPluginBase's contract for some imagineable custom plugins of that type. Is it likely? No. But it doesn't hurt to stay BC.

wizonesolutions’s picture

wizonesolutions
he/him
Primary language English
Location Norway
CreditAttribution: wizonesolutions as a volunteer and at WizOne Solutions for FillPDF Service commented
Assigned: wizonesolutions » Unassigned

I'm half-tempted to just remove the interfaces to get people to actually talk to us in the issue queue. But I agree with you. Whether intentional or not, the interfaces are hinted, so they should stay there and not break. We have to fix some of my earlier design decisions in 8.x-5.x.

Pancho’s picture

Pancho
Location UTC+2 🇪🇺 EU
CreditAttribution: Pancho as a volunteer commented
FileSize
3058957-5.patch8.85 KB
3058957_3-5_interdiff.txt930 bytes

We also need to temporarily override PluginBase::isConfigurable() as we're no longer extending ConfigurablePluginInterface. We may however only implement ConfigurableInterface once we're no longer supporting Drupal 8.6.x. So adding a couple of @todos and fixing deprecation notices.

Pancho’s picture

Pancho
Location UTC+2 🇪🇺 EU
CreditAttribution: Pancho as a volunteer commented
FileSize
fillpdf_action_plugins_3058862_9.patch5.08 KB
3058862_3-9_interdiff.txt4.09 KB

Please ignore the patch in #9. Here's the correct one.

Pancho’s picture

Pancho
Location UTC+2 🇪🇺 EU
CreditAttribution: Pancho as a volunteer commented
FileSize
fillpdf_action_plugins_3058862_11.patch5.07 KB
3058862_9-11_interdiff.txt4.08 KB

Just some coding standards.

  • Pancho committed 7185e45 on 8.x-4.x 
    Issue #3058862 by Pancho: Avoid using deprecated...
Pancho’s picture

Pancho
Location UTC+2 🇪🇺 EU
CreditAttribution: Pancho as a volunteer commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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