Allow backend plugins to supply settings for the FillPdfSettingsForm

This is also necessary to allow for proper UI testing of the FillPdfSettingsForm, as it will otherwise complain about 'test' being an illegal choice.

YAY!

Backend plugins are now configurable, support inspection, have forms and are even aware of other plugins (local_service) or provide dependencies (fillpdf_service). :)

And this while legacy plugins don't just keep working, but are detected and selectable, so even for those ones no more form_alter unless you need configuration...

Hope I can finish this one by tomorrow, but can't promise.

Here's a first patch that reimplements the backend plugins as v5 API while making sure legacy implementations keep working basically unchanged.

Tested green locally, so normally the testbot should be happy, too.

Unfortunately ATM I don't have time to explain design decisions and everything, so I can only post this as-is for now and will followup on it by Monday morning. But feel free to test all backends and experiment with it! :)

These are just PHP5 compatibility issues, no big deal.

Raising priority to major as this is both a major effort and an important step forward.

• fixed signature of testBackend() which PHP5 complained about
• switched $messenger and $logger_factory parameters in plugin constructor
• fixed v5 test plugin annotation
• some more coding standards fixes

Tests green now with both PHP5 and PHP7. :)

You based the function signatures on FillPdfBackend. Please base them on BackendService instead (parse/merge).

Not exactly. LOL, yes, but that doesn't have to be.

After some back and forth I decided to replace both FillPdfBackend and LocalService with the new PdfBackend plugins. Basing them all on BackendService would still require the exactly same amount of BC layer, and might still break BC with custom BackendService plugins and LocalService implementations.

Furthermore, while we're redoing backend plugins, "PdfBackend" seems the better name. For one thing, the plugins themselves are no services, only the plugin manager is. Secondly, the ubiquitious use of "service" is confusing - there would be a "FillPDF Service" backendService, a "Local Service" backend service and a "pdftk" backend service, neither of which really is a service in the Drupal sense. The naming scheme I'm proposing seems superior regarding both clarity and brevity.

However: Regarding parse($pdf) and merge($pdf), I left the code in the deprecated LocalService backend service for now, as I wanted to figure out with you how to do it best. In the end, I would like to have as much as possible encapsulated in a single plugin file, at least everything related to a particular backend. So my proposal would indeed be to merge the two functions without signature change into LocalServiceBackend. As even with PHP7 there's still no overloading, the other parse($fillpdf_form) would then have to be renamed, possibly to parseForm($fillpdf_form). This should already be possible in the experimental v5 API of the 8.x-4.x (and possibly 7.x-3.x) branch.

So yeah, in the end your local implementation would have to use the new plugin manager to instantiate the plugin, but then everything would be unchanged. In 8.x-5.x, when getting rid of BC, we could even switch back to the old plugin manager's service name, so from the outside nothing would be different from today: call the same plugin manager service, instantiate a plugin with the same id, call the same old parse() and merge() functions with the same old signatures.

I will likely make more changes when I look closer.

Absolutely. Some aspects clearly need to be thought through, worked out and/or changed.

I just meant I wanted to call the functions parse and merge, not parse and populateWithFieldData.

Simple and short answer: yes, but I think it would make sense to have both on every plugin:
parse($pdf), 1:1 from the legacy BackendService plugin.
merge($pdf), 1:1 from the legacy BackendService plugin.
parseForm($fillpdf_form), formerly parse($fillpdf_form) on the legacy FillPdfBackend plugins.
mergeForm($fillpdf_form), formerly populateWithFieldData($fillpdf_form) on the legacy FillPdfBackend plugins.

Finally, your external code might prefer supplying a PDF, while our Drupal code has the FillPdfForm entity and doesn't (want to) care about anything else.

Alternatively we could imitate overloading by avoiding parameter typing and then checking what it is. I don't like that pattern all too much, but as a first step it might be the easiest thing to do.

But then again, external code shouldn't use our plugins directly. There should be some shielding abstraction anyway.

So it's your call how you'd like the signatures to be within this issue's scope. :)

Another thought:

Actually, the backend plugin shouldn't care about how to retrieve a File object from a FillPdfForm.
It should take wither a file string or a File object to parse() and merge() it as it's being told, and then return either a file string or a File object.

Unsure which would be better, as the plugin might need some file metadata, or if we should allow both, but that may still be figured out.

Fact is we should go with parse($file) and merge($file), while it remains to be determined which type of $file they should take.

TLDR:
Yes, you were right in #9:
The function signatures of parse() and merge() should be based on BackendService's example, not FillPdfBackend's example.
:D

1.
Should testBackend() be just test()? I think so.

2.
Don't know yet what to do with / how to best implement isAvailable() on the plugin and getAvailableBackends() on the manager. Alternatively we could implement FilteredPluginManagerInterface, though that might be a bit overkill. I want the plugins to know their status though.

3.
ExecutableManagerInterface should probably go from the manager as well, as the plugins aren't executable in the conventional sense. Or are they? I need to think it through.

Here's a first step towards #9 and #13. Now at least all code is moved into the new plugins. We still need to refactor and rename.

Why did I switch the issue back to normal priority? I guess, erroneously.
Still needs work though, including the points raised in #14.

The private $httpClient property may go, too. The protected ones I would leave in and the signature I would leave as is for now. Someone might be extending the class.

Same x2.

Yes, you were right in #9:
The function signatures of parse() and merge() should be based on BackendService's example, not FillPdfBackend's example.

1.)
OK, this tiny step was quite a bit complicated. changing the signature of parse() required completely decoupling the new interface from the old one (otherwise PHP would complain the parent interface's contract was broken). In the end, it seems to work fine again.

However:

Fact is we should go with parse($file) and merge($file), while it remains to be determined which type of $file they should take.

Indeed, unlike the other backends, the pdftk backend needs the \Drupal\file\Entity\File object (actually, just the URI), not the file's actual content (as returned by file_get_contents()). So BackendService's parse(string $content) and merge(string $content) signatures wouldn't immediately work for pdftk.

There should be a way to make it work, however I believe it is absolutely correct that parse() and merge() take \Drupal\file\Entity\File objects now and do with them whatever they need.

This is the difference it makes:

  /**
   * {@inheritdoc}
   */
  public function parse(File $file) {
    $pdf_content = file_get_contents($file->getFileUri());
    return $this->_parse($pdf_content);
  }

Is this acceptable from the perspective of your local code?

2.)
Then let's go to the next tricky aspect:

Drupal\fillpdf\Plugin\BackendService\LocalService::merge() used to take an array of FieldMapping objects while all other implementations used to take the $field_mapping['fields'] style arrays. I don't know which one is better (objects usually are) and if we can make it work differently.

For the moment, I stuck with the least common denominator which is the $field_mapping['fields'] style arrays. \Drupal\fillpdf\Plugin\PdfBackend\LocalServiceBackend::merge() would still create FieldMapping objects (only to pull them apart a few lines later) while the other implementations still wouldn't.

If it makes a lot of sense, we can create FieldMapping objects outside and feed them to the plugins. I'm just unsure ATM, if it's worth it. A closer code review might tell, but at this point, your input would be even more valuable. :)

Don't worry about my backend code. Again, I only meant the function names.

Ah, okay, that's quite comforting! :)

Files or strings rather than FillPDF forms do seem better, as it then lets third parties use the functionality without needing to fake a FillPDF form.

Exactly, that's what I thought.

So all we still need to figure out is how strong we're feeling about creating FieldMapping objects. Again, ATM I have no opinion on them. For now, we might want to stick with what we have in #18 though (apart from the three @todos mentioned in #14 and possibly a few more minor changes). The v5 plugin API doesn't have to be final before a 8.x-5.x release is rolled out, so my primary focus would be more on some automated and manual testing, whether the BC layer does indeed work with FillPDF Service and FillPDF LocalServer.

In the meantime, I'm trying to tear out all/most non-API breaking changes to a separate issue, so to make #18 easier to review. We would then:

1. commit those non-API breaking changes right away (with tests, of course)
2. then fix most other more important outstanding issues in the tracker (and possibly some more bugs)
3. then run tests with PHP 5.5, making sure we're still compatible
4. then do a proper 8.x-4.8 release
5. then enforce a minimum of PHP 5.6
6. then come back reroll #18, do a proper code review and fix what's missing
7. then go ahead with committing this issue to 8.x-4.x dev

Files or strings rather than FillPDF forms do seem better, as it then lets third parties use the functionality without needing to fake a FillPDF form.

Exactly, that's what I thought.

Oh, and that's probably what you thought a year ago, when you implemented LocalService. Not claiming that idea. :)

In the meantime, I'm trying to tear out all/most non-API breaking changes to a separate issue, so to make #18 easier to review. We would then:
1. commit those non-API breaking changes right away (with tests, of course)

I spun off #3044666: Autodetect and present all available backend plugins and #3044678: Fix coding style issues all around backend plugins and will post patches for these two spin-offs. However, I won't commit them before tonight. While they will help us focus this issue on the actual API changes, they will break the currently working patch in #18, so manual testing would only become possible again after a manual reroll.

Needs a reroll after #3044678: Fix coding style issues all around backend plugins. Also blocked on #3044666: Autodetect and present all available backend plugins.

#3044666: Autodetect and present all available backend plugins is in, so while still in need of a reroll, it's no longer blocked.

Rerolled after #3044678: Fix coding style issues all around backend plugins and #3044666: Autodetect and present all available backend plugins, which helped us focus more on the main issue here. So even if the patch still amounts to almost 100k, it's way easier to read and review than the one in #18.

Due to #3044666, we're also in the comfortable situation to have substantially expanded test coverage now, so while we definitely need some more backend-specific testing (both manually and automated), I'm confident that we won't be breaking anything.

Creating an interdiff vs. #18 turned out to be almost impossible, but we're starting the review process only at this point anyway.

#9 is fixed. Still needs work for #14 and a couple more aspects.

Quite some mostly minor fixes (codestyle and others).

Another reroll following #3044868: Don't save unvalidated backend configuration if another backend is chosen..

Thanks for your review and all valuable comments!

Mostly just nitpicks and naming stuff I'm this.

Nitpicks are important, so that's fine!

the "BC switch" module fillpdf_v5, I'm still not entirely sure if it's necessary. Not having it wouldn't break the old plugins, and people shouldn't be relying on FillPDF using specific plugins within its implementation code.

Indeed, changing the plugin's implementation wouldn't be a problem. However, for the new plugins we're using (and unfortunately have to use) a new PluginManager that only picks up the new plugins, while the old PluginManager only picks up the old plugins.

So the BC layer is more to allow for switching between the PluginManagers rather than for the Plugins themselves. Too bad, Core won't allow for picking up plugins from different directories unless they have a $vendor directory in common, see the last section of Create your own custom annotation class which I actually wrote after trying to make it work without a BC layer...

Postponed on #3048947: Refactor HandlePdfController::populatePdf(), which will let us get as far as possible w/o breaking BC.

In this issue here, we may then focus on what isn’t possible w/o breaking BC.

Still postponed on #3060086: Introduce new PdfBackend plugins to replace legacy FillPdfBackend and BackendService plugins, but no longer on the 8.x-5.x branch.

No longer postponed.

Next patch will be massively smaller as #3060086: Introduce new PdfBackend plugins to replace legacy FillPdfBackend and BackendService plugins already introduced the new PdfBackend plugins.

Finally back on track. Here's a new patch. Not really massively smaller, but a bit.

It is manually tested to work fine, locally tests green, and is a much, much better solution than everything we had before:
We're having full freedom to redesign our v5 plugin API while providing almost full BC to existing installs. At the same time I managed to get most of the BC layer out of the way, for new installs to be clean and get ready for 8.x-5.x.

The whole issue cost me far too much time, so I'd really love to get it over and done. But still, we should do it right, so I'd love to see some review and am ready to discuss and even bikeshed how to do it best. Otherwise, we can still do followups, but that's only second best.

Major reroll after #3040900: Move legacy backend support incl. the JavaBridge ("local") backend to a submodule.
Also split #3063023: Mark required backend settings #required off.

Result is a much cleaner, smaller patch that tested green locally, should pass here, too, and be ready for review then.

Assigning to @wizonesolutions for a thorough review.

Note that I didn't rename the test() method as I wasn't 100% sure about either name. Some of the options were ready() or isReady() or check(), but I'd like to study Core and forthcoming Core patterns a bit more.

Here's a rather straight reroll against 8.x-4.x-dev which should also be ready for 5.0.x, just with deprecated stuff still in. Let's see if it goes through.

And yay, I'm back! ;)

Let's try separate patches.

Too bad my testbed isn't prepared yet, so I could sort out these errors rightaway.
Let's try again, anyway.

Next try.