Drupal 9 Deprecated Code Report

Here is the patch, please review.

Please review patch

#4 was against 1.x-Branch I think, so I worked with #2. Hopefully the tests come back green. There are still deprecation notices left, and simpletests which have to be converted.

Trying to convert the tests.

Hi @maacl

I tried patch #7 with drupal-check and looks OK, do you have any example about how to convert the test and what tests needs to be converted, I could use that information to try to help with this task.

I hadn't had the chance to do a deep dive into testing yet. As far as I can tell, there are tests using simpletest classes and traits. Those have to be replaced by PHPunit equivalents. Some tests may need rewriting. You can find them with a simple search in the codebase: https://git.drupalcode.org/search?utf8=%E2%9C%93&search=simpletest&group...

Here is a small piece of documentation: https://www.drupal.org/docs/8/testing/converting-d7-and-d8-simpletests-t...

Fixed tests result from EntityPrintAdminTest of last patch.

Just did a super-quick scan of #10 and noticed it already has core_version_requirement: ^8 || ^9, so I think it's a good idea to test it against both 8.9.x and 9.0.x.

Here are some new code deprecation issues. This is from 4592581.

 ------ --------------------------------------------------------------------
  Line   src/Plugin/EntityPrint/PrintEngine/DomPdf.php
 ------ --------------------------------------------------------------------
  69     Call to deprecated function file_directory_temp():
         in drupal:8.8.0 and is removed from drupal:9.0.0. Use
         \Drupal\Core\File\FileSystemInterface::getTempDirectory() instead.
  70     Call to deprecated function file_directory_temp():
         in drupal:8.8.0 and is removed from drupal:9.0.0. Use
         \Drupal\Core\File\FileSystemInterface::getTempDirectory() instead.
  71     Call to deprecated function file_directory_temp():
         in drupal:8.8.0 and is removed from drupal:9.0.0. Use
         \Drupal\Core\File\FileSystemInterface::getTempDirectory() instead.
  72     Call to deprecated function file_directory_temp():
         in drupal:8.8.0 and is removed from drupal:9.0.0. Use
         \Drupal\Core\File\FileSystemInterface::getTempDirectory() instead.
 ------ --------------------------------------------------------------------

 ------ -----------------------------------------------------------------------------------------------
  Line   src/Plugin/ExportTypeManager.php
 ------ -----------------------------------------------------------------------------------------------
  15     Drupal\entity_print\Plugin\ExportTypeManager must override __construct if using YAML plugins.
 ------ -----------------------------------------------------------------------------------------------

 ------ ------------------------------------------------------------------------------------
  Line   src/Plugin/PrintEngineInterface.php
 ------ ------------------------------------------------------------------------------------
  12     Interface Drupal\entity_print\Plugin\PrintEngineInterface extends deprecated interface
     Drupal\Component\Plugin\ConfigurablePluginInterface:
         Drupal\Component\Plugin\ConfigurablePluginInterface is deprecated
         in Drupal 8.7.0 and will be removed before Drupal 9.0.0. You should implement
         ConfigurableInterface and/or DependentPluginInterface directly as needed. If
         you implement ConfigurableInterface you may choose to implement
         ConfigurablePluginInterface in Drupal 8 as well for maximum compatibility,
         however this must be removed prior to Drupal 9.
 ------ ------------------------------------------------------------------------------------

 ------ -------------------------------------------------------------------------------------------------------------------
  Line   src/PrintBuilder.php
 ------ -------------------------------------------------------------------------------------------------------------------
  99     Call to deprecated constant FILE_EXISTS_REPLACE: Deprecated in drupal:8.7.0 and is removed from drupal:9.0.0. Use
         \Drupal\Core\File\FileSystemInterface::EXISTS_REPLACE.
  99     Call to deprecated function file_unmanaged_save_data():
         in drupal:8.7.0 and is removed from drupal:9.0.0.
         Use \Drupal\Core\File\FileSystemInterface::saveData().
 ------ -------------------------------------------------------------------------------------------------------------------

 ------ ---------------------------------------------------------------------------------------------------------------
  Line   src/Tests/EntityPrintActionTest.php
 ------ ---------------------------------------------------------------------------------------------------------------
  12     Class Drupal\entity_print\Tests\EntityPrintActionTest extends deprecated class Drupal\simpletest\WebTestBase:
         in drupal:8.8.0 and is removed from drupal:9.0.0. Instead,
         use \Drupal\Tests\BrowserTestBase. See https://www.drupal.org/node/3030340.
  25     Call to method setUp() of deprecated class Drupal\simpletest\WebTestBase:
         in drupal:8.8.0 and is removed from drupal:9.0.0. Instead,
         use \Drupal\Tests\BrowserTestBase. See https://www.drupal.org/node/3030340.
 ------ ---------------------------------------------------------------------------------------------------------------

 ------ --------------------------------------------------------------------------------------------------------------
  Line   src/Tests/EntityPrintAdminTest.php
 ------ --------------------------------------------------------------------------------------------------------------
  13     Class Drupal\entity_print\Tests\EntityPrintAdminTest extends deprecated class Drupal\simpletest\WebTestBase:
         in drupal:8.8.0 and is removed from drupal:9.0.0. Instead,
         use \Drupal\Tests\BrowserTestBase. See https://www.drupal.org/node/3030340.
  33     Call to method setUp() of deprecated class Drupal\simpletest\WebTestBase:
         in drupal:8.8.0 and is removed from drupal:9.0.0. Instead,
         use \Drupal\Tests\BrowserTestBase. See https://www.drupal.org/node/3030340.
 ------ --------------------------------------------------------------------------------------------------------------

 ------ ----------------------------------------------------------------------------------------------
  Line   tests/src/Kernel/PostRenderSubscriberTest.php
 ------ ----------------------------------------------------------------------------------------------
  40     Call to deprecated method setExpectedException() of class Drupal\KernelTests\KernelTestBase:
         in drupal:8.8.0 and is removed from drupal:9.0.0.
         Backward compatibility for PHPUnit 4 will no longer be supported.
 ------ ----------------------------------------------------------------------------------------------

Please review the patch.

please review the code.

@karishmaamin: Your patch is incomplete and is not based on the existing work here, reviewing #13.

1. +++ b/src/EventSubscriber/PrintEngineExceptionSubscriber.php
   @@ -30,6 +30,12 @@ class PrintEngineExceptionSubscriber implements EventSubscriberInterface {
       */
      protected $entityTypeManager;
   +  /**
   

   needs an empty line here.

2. +++ b/src/EventSubscriber/PrintEngineExceptionSubscriber.php
   @@ -39,9 +45,10 @@ class PrintEngineExceptionSubscriber implements EventSubscriberInterface {
       * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entityTypeManager
       *   Entity type manager.
       */
   -  public function __construct(RouteMatchInterface $routeMatch, EntityTypeManagerInterface $entityTypeManager) {
   +  public function __construct(RouteMatchInterface $routeMatch, EntityTypeManagerInterface $entityTypeManager,MessengerInterface $messenger ) {
        $this->routeMatch = $routeMatch;
        $this->entityTypeManager = $entityTypeManager;
   +    $this->messenger = $messenger;
      }
    
      /**
   

   missing space, missing @param and looks like several of these services aren't updated in services.yaml for the new arguments.

3. +++ b/src/Form/SettingsForm.php
   @@ -235,7 +235,7 @@ class SettingsForm extends ConfigFormBase {
    
   -    drupal_set_message($this->t('Configuration saved.'));
   +    $this->messenger->addStatus($this->t('Configuration saved.'));
      }
   

   should use messenger()

4. +++ b/src/PrintBuilder.php
   @@ -33,6 +34,13 @@ class PrintBuilder implements PrintBuilderInterface {
    
      /**
   +   * The event dispatcher.
   +   *
   +   * @var \Drupal\Core\File\FileSystemInterface
   +   */
   +  protected $fileSystem;
   +
   +  /**
       * Constructs a new EntityPrintPrintBuilder.
   

   this is not the event dispatcher.

5. +++ b/src/Tests/EntityPrintActionTest.php
   @@ -2,14 +2,14 @@
    
    namespace Drupal\entity_print\Tests;
    
   -use Drupal\simpletest\WebTestBase;
   +use Drupal\Tests\BrowserTestBase;
    
    /**
     * Test the Entity Print action tests.
     *
     * @group entity_print
     */
   -class EntityPrintActionTest extends WebTestBase {
   +class EntityPrintActionTest extends BrowserTestBase {
    
      /**
   

   The test classes aren't moved to the new, correct location (test/src/Functional)

   They are also missing the new required $defaultTheme property.

Also, .info.yml files aren't updated and the test theme should have a base theme property, see fails in https://www.drupal.org/pift-ci-job/1648706.

Please review the patch.

I'll try to merge the different patches into one.

I combined #10, #15 and #17 and included the feedback from #16. Thanks Berdir! I hope this will apply.

Missed one service, and fixed the code style. Interdiff again for #17.

+++ b/src/Plugin/EntityPrint/PrintEngine/DomPdf.php
@@ -58,18 +59,25 @@ class DomPdf extends PdfEngineBase implements ContainerFactoryPluginInterface {
 
-    $this->dompdfOptions->setTempDir(file_directory_temp());
-    $this->dompdfOptions->setFontCache(file_directory_temp());
-    $this->dompdfOptions->setFontDir(file_directory_temp());
-    $this->dompdfOptions->setLogOutputFile(file_directory_temp() . DIRECTORY_SEPARATOR . self::LOG_FILE_NAME);
+    $this->dompdfOptions->setTempDir($this->fileSystem->getTempDirectory());
+    $this->dompdfOptions->setFontCache($this->fileSystem->getTempDirectory());
+    $this->dompdfOptions->setFontDir($this->fileSystem->getTempDirectory());
+    $this->dompdfOptions->setLogOutputFile($this->fileSystem->getTempDirectory() . DIRECTORY_SEPARATOR . self::LOG_FILE_NAME);
     $this->dompdfOptions->setIsRemoteEnabled($this->configuration['enable_remote']);

this is a Drupal 8.8 deprecation, that means the core_version_requirement needs to say ^8.8 || ^9 and core: 8.x needs to be removed.

> Behat\Mink\Exception\ExpectationException: The string "node--view-mode-full" was not found anywhere in the HTML response of the current page.

This fails because this class doesn't exist in stark. The easiest fix for now is to set $defaultTheme to classy instead for that test.

Thanks, working on it.

+++ b/src/Plugin/EntityPrint/PrintEngine/DomPdf.php
@@ -58,18 +59,25 @@ class DomPdf extends PdfEngineBase implements ContainerFactoryPluginInterface {
 
-    $this->dompdfOptions->setTempDir(file_directory_temp());
-    $this->dompdfOptions->setFontCache(file_directory_temp());
-    $this->dompdfOptions->setFontDir(file_directory_temp());
-    $this->dompdfOptions->setLogOutputFile(file_directory_temp() . DIRECTORY_SEPARATOR . self::LOG_FILE_NAME);
+    $this->dompdfOptions->setTempDir($this->fileSystem->getTempDirectory());
+    $this->dompdfOptions->setFontCache($this->fileSystem->getTempDirectory());
+    $this->dompdfOptions->setFontDir($this->fileSystem->getTempDirectory());
+    $this->dompdfOptions->setLogOutputFile($this->fileSystem->getTempDirectory() . DIRECTORY_SEPARATOR . self::LOG_FILE_NAME);
     $this->dompdfOptions->setIsRemoteEnabled($this->configuration['enable_remote']);

@berdir I think you may have had to say something about that. This are deprecations of DomPDF and may be out of scope.

this is a Drupal 8.8 deprecation, that means the core_version_requirement needs to say ^8.8 || ^9 and core: 8.x needs to be removed.

Updating Drupal Core to 8.8 removes `core: 8.x` from configuration also, so I will remove it here, too.

Behat\Mink\Exception\ExpectationException: The string "node--view-mode-full" was not found anywhere in the HTML response of the current page.

This fails because this class doesn't exist in stark. The easiest fix for now is to set $defaultTheme to classy instead for that test.

Ah, thank you. Trying to learn this stuff.

I tried to figure out what is happening in the failing tests, but did not manage to do so. The EntityPrintAdminTest may need futher conversion to a test with Javascript to keep the original behavior. Lets see what comes back from the Testbot.

Intederiff was against wrong patch, here the fixed one.

I think I figured out the test fail in EntityPrintActionTest::testDownloadPdfAction(). Should be fixed, is passing locally.

I reverted the changes to EntityPrintAdminTest::testAdminSettings() because I do not understand what the test is looking for exactly and why the fileds are missing. Probably because it requires JS.

This is still missing a composer require sectiondrupal/core: "^8 || ^9"

That's optional, drupal.org will add that automatically. Only needs to be done if composer.json already has a line for drupal/core.

With latest dev and latest patch, I still get this:

vendor/bin/drupal-check web/modules/contrib/entity_print/
80/80 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

------ -----------------------------------------------------------------------------------------------
Line src/Plugin/ExportTypeManager.php
------ -----------------------------------------------------------------------------------------------
15 Drupal\entity_print\Plugin\ExportTypeManager must override __construct if using YAML plugins.
------ -----------------------------------------------------------------------------------------------

[ERROR] Found 1 error

False positive ?

The key thing to look at is

if using YAML plugins

This module's plugin manager isn't using yaml plugins, so it doesn't apply.

Still NW though because tests are failing.

Just FYI

CONTRIBUTED PROJECTS
--------------------------------------------------------------------------------
Entity Print 8.x-2.x-dev
Scanned on Sat, 05/23/2020 - 09:26.

3 warnings found.

web/modules/contrib/entity_print/src/Plugin/ExportTypeManager.php:
┌──────────┬──────┬────────────────────────────────────────────────────────────┐
│  STATUS  │ LINE │                          MESSAGE                           │
├──────────┼──────┼────────────────────────────────────────────────────────────┤
│ Check    │ 15   │ Drupal\entity_print\Plugin\ExportTypeManager must override │
│ manually │      │ __construct if using YAML plugins.                         │
│          │      │                                                            │
└──────────┴──────┴────────────────────────────────────────────────────────────┘

web/modules/contrib/entity_print:
┌──────────┬──────┬─────────────────────────────────────────────────────────────┐
│  STATUS  │ LINE │                           MESSAGE                           │
├──────────┼──────┼─────────────────────────────────────────────────────────────┤
│ Check    │ 0    │ The 'entity_print' extension is not installed. Cannot check │
│ manually │      │ deprecated library use.                                     │
│          │      │                                                             │
└──────────┴──────┴─────────────────────────────────────────────────────────────┘

web/modules/contrib/entity_print/src/Asset/AssetRenderer.php:
┌──────────┬──────┬──────────────────────────────────────────────────────────────┐
│  STATUS  │ LINE │                           MESSAGE                            │
├──────────┼──────┼──────────────────────────────────────────────────────────────┤
│ Check    │ 59   │ The 'entity_print/default' library is not defined because    │
│ manually │      │ the defining extension is not installed. Cannot decide if it │
│          │      │ is deprecated or not.                                        │
│          │      │                                                              │
└──────────┴──────┴──────────────────────────────────────────────────────────────┘

CONTRIBUTED PROJECTS
--------------------------------------------------------------------------------
Entity Print 8.x-2.x-dev
Scanned on Sat, 05/23/2020 - 10:36.

1 warning found.

web/modules/contrib/entity_print/src/Plugin/ExportTypeManager.php:
┌──────────┬──────┬────────────────────────────────────────────────────────────┐
│  STATUS  │ LINE │                          MESSAGE                           │
├──────────┼──────┼────────────────────────────────────────────────────────────┤
│ Check    │ 15   │ Drupal\entity_print\Plugin\ExportTypeManager must override │
│ manually │      │ __construct if using YAML plugins.                         │
│          │      │                                                            │
└──────────┴──────┴────────────────────────────────────────────────────────────┘

Confirmed that #37 is the only one left, I should install the module first, before scanning with upgrade_status. The report in #39 was from without installing the module. Sorry for the noisy.

That yaml message is bogus and should be ignored

This needs a reroll, after #3147348: Automated Drupal 9 compatibility fixes has been comitted.

Rerolled -

Another potential issue:

ArgumentCountError: Too few arguments to function Drupal\entity_print\Plugin\EntityPrint\PrintEngine\DomPdf::__construct(), 5 passed in /modules/contrib/entity_print/src/Plugin/EntityPrint/PrintEngine/DomPdf.php on line 103 and exactly 6 expected in Drupal\entity_print\Plugin\EntityPrint\PrintEngine\DomPdf->__construct() (line 70 of modules/contrib/entity_print/src/Plugin/EntityPrint/PrintEngine/DomPdf.php).
Drupal\entity_print\Plugin\EntityPrint\PrintEngine\DomPdf->__construct(Array, 'dompdf', Array, Object, Object) (Line: 103)
Drupal\entity_print\Plugin\EntityPrint\PrintEngine\DomPdf::create(Object, Array, 'dompdf', Array) (Line: 21)
Drupal\Core\Plugin\Factory\ContainerFactory->createInstance('dompdf', Array) (Line: 83)
Drupal\Component\Plugin\PluginManagerBase->createInstance('dompdf', Array) (Line: 90)
Drupal\entity_print\Plugin\EntityPrintPluginManager->createInstance('dompdf') (Line: 186)
Drupal\entity_print\Form\SettingsForm->getPluginForm('dompdf', Object) (Line: 159)
Drupal\entity_print\Form\SettingsForm->buildForm(Array, Object, Object)
call_user_func_array(Array, Array) (Line: 531)
Drupal\Core\Form\FormBuilder->retrieveForm('entity_print_admin_settings_form', Object) (Line: 277)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 91)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 573)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 151)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 68)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 708)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

Hi!

First of all I'm sorry but I don't know how to produce patches.

The module seems to work for me on Drupal 9 after applying the patch #45 and injecting the file system service in the create function in the file modules/contrib/entity_print/src/Plugin/EntityPrint/PrintEngine/DomPdf.php:

public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
    return new static(
      $configuration,
      $plugin_id,
      $plugin_definition,
      $container->get('plugin.manager.entity_print.export_type')->createInstance($plugin_definition['export_type']),
      $container->get('request_stack')->getCurrentRequest(),
      $container->get('file_system')
    );
  }

An other error occured, I had to replace the static function in the renderSingle function in the file modules/contrib/entity_print/modules/entity_print_views/src/Renderer/ViewRenderer.php by an anonymous function:

$render['#pre_render'][] = function (array $element) {
    // Remove the exposed filters, we don't every want them on the PDF.
    $element['#exposed'] = [];
    return $element;
};

@gaspounet Thanks!

I've rerolled the patch with those two changes. It does address the issue I posted earlier, but I think there are still problems with entity_print_views. When attempting to use a "view PDF" option in the view header, I get the following error:

Error generating document: Rendering not yet supported for "Drupal\views\Entity\View". Entity Print context "entity"

Looks like it's coming from src/Renderer/RendererFactory

Okay, put some time into this. New patch attached.

Changes since #49:

1. I've moved core_version_requirement up (where core has been previously)
2. Tests should not install incorrect schema of the system module (router table is not part of the system module) - special handling of system module schemas in \Drupal\KernelTests\KernelTestBase::installSchema has been deprecated in Drupal 8.7.x, remove any calls to this method that use invalid schema names. See [#3003360]
3. Modified tests to use the Stark theme, e.g. let's not depend on the markup, but on the field/label values
4. Modified settings form to include settings of all available plugins (so tests pass & form does work as expected without JS)
5. Fixed Providing context definitions via the "context" key is deprecated in Drupal 8.7.x and will be removed before Drupal 9.0.0. Use the "context_definitions" key instead.

Running locally against D9.1.x:

./vendor/bin/phpunit -c docroot/core/ docroot/modules/contrib/entity_print/
PHPUnit 8.5.5 by Sebastian Bergmann and contributors.

Testing docroot/modules/contrib/entity_print/
...W..................W......W........E...WW...W....              52 / 52 (100%)

So, new patch against D9.1.x:

$ ./vendor/bin/phpunit -c docroot/core/ docroot/modules/contrib/entity_print/
PHPUnit 8.5.6 by Sebastian Bergmann and contributors.

Testing docroot/modules/contrib/entity_print/
....................................................              52 / 52 (100%)

Time: 6.04 minutes, Memory: 16.00 MB

OK (52 tests, 455 assertions)

Remaining self deprecation notices (2)

  2x: The "Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent::getException()" method is deprecated since Symfony 4.4, use "getThrowable()" instead.
    1x in EntityPrintTest::testExceptionOnRender from Drupal\Tests\entity_print\Functional
    1x in EntityPrintTest::testViewsExceptionOnRender from Drupal\Tests\entity_print\Functional

Remaining deprecation cannot be fixed atm, as this deprecation is coming from Symfony 4.3 (unless D8 support is no longer a goal)

Changes since #50:

1. Fixed Declaring ::setUp without a void return typehint in Drupal\Tests\entity_print\Kernel\NodePreviewTest is deprecated in drupal:9.0.0. Typehinting will be required before drupal:10.0.0. See [#3114724]
2. Replaced assertContains()/assertNotContains() with assertStringContainsString()/assertStringNotContainsString()
3. Replaced @expectedException, @expectedExceptionCode, @expectedExceptionMessage, and @expectedExceptionMessageRegExp with expectException(), expectExceptionCode(), expectExceptionMessage(), or expectExceptionMessageMatches().
4. Replaced deprecated asserts (btw they are not triggered by tests)

Oops, incorrect interdiff, fixed

And another small update: test modules/themes do not need to specify supported core versions and let's have stark everywhere

Trying something

hi @Sam152 - it does not work, I was trying to achieve the same in #50

In order to be able to run D9 on DrupalCI, core_version_requirement must be committed first

Okay, interesting.

I can commit this, but I do have some questions:

1. +++ b/modules/entity_print_views/src/Renderer/ViewRenderer.php
   @@ -52,7 +52,11 @@ class ViewRenderer extends RendererBase {
   -    $render['#pre_render'][] = [static::class, 'preRender'];
   +    $render['#pre_render'][] = function (array $element) {
   +      // Remove the exposed filters, we don't every want them on the PDF.
   +      $element['#exposed'] = [];
   +      return $element;
   +    };
   

   Why can't we use the existing prePrender method? Lets just implement TrustedCallbackInterface?

2. +++ b/src/Form/SettingsForm.php
   @@ -155,8 +155,12 @@ class SettingsForm extends ConfigFormBase {
   -      if ($this->pluginManager->isPrintEngineEnabled($selected_plugin_id)) {
   -        $form['entity_print'][$export_type . '_config'][$selected_plugin_id] = $this->getPluginForm($selected_plugin_id, $form_state);
   +      if (!empty($print_engines[$export_type])) {
   +        foreach ($print_engines[$export_type] as $plugin_id => $label) {
   +          if ($this->pluginManager->isPrintEngineEnabled($plugin_id)) {
   +            $form['entity_print'][$export_type . '_config'][$plugin_id] = $this->getPluginForm($plugin_id, $form_state);
   +          }
   +        }
   

   Why was all this added?

3. +++ b/src/PrintBuilder.php
   @@ -97,7 +105,7 @@ class PrintBuilder implements PrintBuilderInterface {
   -    return \Drupal::service('file_system')->saveData($print_engine->getBlob(), $uri, FileSystemInterface::EXISTS_REPLACE);
   +    return $this->fileSystem->saveData($print_engine->getBlob(), $uri, FileSystemInterface::EXISTS_REPLACE);
   

   Updating these to all use dependency injection seems kind of out of scope? Wasn't required for D9 support right?

I've committed the core_version_requirement changes only, to get a test run working. I'm going to try post a patch with the completely unambiguous stuff to see how far that gets.

Looks like injecting the messenger service was cosmetic as well.

Hi @Sam152- I can answer to point 2:

So, the Drupal\Tests\BrowserTestBase does run tests slighly differently than Drupal\simpletest\WebTestBase in the context how AJAX is handled: BrowserTestBase does not emulate AJAX calls in any way. So, tests run like a page without JS.

The current implementation of the form does not work without JS, because settings of the selected plugin are present in the form array only.

What happens:

The form is being built without any plugin config available in the form array because all export types are not set. Then you select plugin for PDF; press Save configuration; then the form is validated and submitted. Submit callback saves the configuration of the selected plugin ID, but it's empty, so the default values are being deleted. This change adds all existing/default config to the form, so it does not matter which plugin will be selected - it's settings will be preserved.

This change adds all existing/default config to the form, so it does not matter which plugin will be selected - it's settings will be preserved.

Okay, this is something that shouldn't be changed as part of this issue. We should instead be using WebDriverTestBase to correctly test AJAX, leaving the functionality intact.

@Sam152 what is the point of removing injections? Backward compatibility? Then add a deprecation with a fallback to the \Drupal call

BTW, the core key should be removed from the info files in order to make it compatible with D9.

Then there are a few places where this change should be also made:

core key can be removed for the testing modules/themes as of 8.8.2:

• https://git.drupalcode.org/project/entity_print/-/blob/8.x-2.x/tests/mod...
• https://git.drupalcode.org/project/entity_print/-/blob/8.x-2.x/tests/the...

core key should be removed and replaced with core_version_requirement:

• https://git.drupalcode.org/project/entity_print/-/blob/8.x-2.x/entity_pr...
• https://git.drupalcode.org/project/entity_print/-/blob/8.x-2.x/modules/e...

Okay, this is something that shouldn't be changed as part of this issue. We should instead be using WebDriverTestBase to correctly test AJAX, leaving the functionality intact.

Well, imho the point of the test is to test functionality, not the AJAX (as it's covered by core tests). A separate JS test can be added in order to test that AJAX is also working. But it's up to you.

The 'core' key can be kept in place, to retain support for running the module on 8.7 and lower.

Well, imho the point of the test is to test functionality, not the AJAX

I agree, but the functionality was changed in the patch? Nobody proposed, discussed or agreed to that, so making the change because it's a bit harder to test in this issue is pretty confusing and should be out of scope.

+++ b/tests/src/Functional/EntityPrintActionTest.php
@@ -49,11 +54,11 @@ class EntityPrintActionTest extends WebTestBase {
-    $this->assertText('Using testprintengine');
+    $this->assertSession()->pageTextContains('Download PDF was applied to 1 item.');

Wondering why this was changed? This indicates to me there was a genuine test failure? The fix should be addressing the failure, not updating the assertions in the test.

Addressing some final bits of feedback on the patch.

Rereading some of the feedback in this issue:

this is a Drupal 8.8 deprecation, that means the core_version_requirement needs to say ^8.8 || ^9 and core: 8.x needs to be removed.

So it looks like 'core' should be removed after all.

This is still missing a composer require sectiondrupal/core: "^8 || ^9"
...
That's optional, drupal.org will add that automatically. Only needs to be done if composer.json already has a line for drupal/core.

Looks like the changes in composer.json can go too.

Somehow credit dropped off for everyone on this issue, trying to recreddit faithfully. For folks who assign and unassign issues to themselves, please stop.

Thank you for the last commit ;) Cheers!

@Sam152, thanks for the quick turnaround and the new stable version!