Resolve issues reported by PHPStan

I've created an MR where PHPStan errors will fail the GitLab CI run. Please contribute fixes on that branch so we can work towards getting a fully passing build and see it "green" on the MR

Ah, so many tools, so many complaints :)

The remaining item has two possible solutions. I defer to the maintainers.

The package NigelCunningham\Puphpeteer is not installed because the dependency is listed in a secondary composer.jon file under puphpeteer. I believe every project is limited to a single composer.json file.

The dependency can be added to the main composer.json file, and the extra composer.json removed.

OR

The Puphpeteer sub-module can be spun off as another project. The namespace is available: https://www.drupal.org/project/puphpeteer →

Leaving the Puphpeteer sub-module as part of the Pdf API project poses another problem. It uses the namespace `Drupal\puphpeteer` which would cause a problem if someone registers https://www.drupal.org/project/puphpeteer → . One solution to this is to prefix it with pdf_api `Drupal\pdf_api_puphpeteer`

I think the easier solution is to spin Puphpeteer off as a separate project.

===========================================================

This is saying there should be hook for an alter function, e.g. hook_pdf_api_generator_alter. We just need to invoke: `$this->alterInfo('pdf_api_generator');`

It wants a cache backend. I gave it the database cache. It seems like a safe option. There is also memory and null cache backends as options.

 ------ ---------------------------------------------------- 
  Line   src/PdfGeneratorPluginManager.php                   
 ------ ---------------------------------------------------- 
  12     Plugin definitions cannot be altered.               
  32     Missing cache backend declaration for performance.  
 ------ ---------------------------------------------------- 
 

@bluegeek9 thanks for laying out those options. We could also choose to suppress those two remaining errors if we don't like either of the options you laid out.

What do you think @Nigel?

I think whatever we do, we should do it consistently - I believe most users tend to use mPDF or domPDF or ...

Perhaps the best idea would be to spin all of the libraries off to separate projects? They could be a pdf_api_* namespace?

I understand the desire for consistency (I share it!), but if all the project would consist of is a wrapper around a composer requirement, that seems heavy-handed to me.

It does make me take a second look at our Composer requirements and realize that anyone who uses this module is being forced to install all four of the PDF libraries listed there. The "pro" of that is it makes setup a relative breeze, but if we are being pedantic maybe those should be moved to a suggest block. Two big cons to this would be that it might make the user experience worse and we wouldn't be able to explicitly declare version constraints.

I am kind of inclined to leave things be on this.

Why is Puphpeteer is separate module? It could be merged with Pdf API.

I believe having four or five PDF libraries is not a problem. They are small in size, and makes setup easier.

Adding NigelCunningham\Puphpeteer to pdf API composer.json will fix the issue and allow site builders to install puphpeteer without having to manually install the dependency.

I have pushed a change that (for now) ignores the two remaining errors that relate to puphpeteer. I propose that we discuss moving puphpeteer or modifying the Composer requirements in a separate issue as this could have implications with regard to user's upgrade path.

If we are all in agreement, we can commit this as-is.

It is a separate module because when I first created it, I wasn't even sure that what I was seeking to do would work (Puppetteer integration was just part of something much bigger). Now that it's all complete and in production, yes - we could just make it another part of the pdf_api module.

That said, I don't think everyone is going to want puppeteer support automatically installed any more than I want/need domPDF, mPDF and wkhtmltoPDF for the Puppeteer project, and Puppeteer support doesn't just imply the Rialto and Puphpeteer modules in the vendor directory. It also needs node, Puppeteer and Chrome themselves, which should really be installed outside of the docroot.

At this stage it sounds wiser and perhaps safer (security issue in an upstream library, anyone?) to me to give the end user the choice regarding what libraries they install.

> I understand the desire for consistency (I share it!), but if all the project would consist of is a wrapper around a composer requirement, that seems heavy-handed to me.

I don't think any of them would just be a composer requirement. Each has a plugin definition and I think the non-puppeteer ones could also perhaps do with a config form to handle the different possibilities for accessing images (chroot jail etc) that still seem to cause people issues with getting images into their PDFs.

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