Drupal9 Readiness and compatibility - Devel

Are you sure we should be adding this now, given that the 2.x branch has 252 deprecation warnings for using code which will not exist in Drupal 9?

In #3042575: [META] Deprecation issues in devel module I have been working on the 3.x branch which is now down to 63 warnings.

I too was working on issues against the 3.x branch whose META parent is on #4 above( https://www.drupal.org/project/devel/issues/3042575)

Yes, salah1 and I have been working only on the 3.x branch for the deprecation issues. I was assuming that we would not port all the changes to the 2.x branch as (a) that is a lot of work, and (b) it will not be needed if we make the decision that only the 3.x branch is taken over in Drupal9. That decision has probably not been made yet. However, it is wrong to commit the patch in #2 which effectively says that 2.x is compatible with D9 when it is far from being so.

There were some more .info.yml files in the repository.

As a reminder, the "assigned" option is for indicating that you're actively working on something, it's then customary to set it back to "unassigned" once you've completed the intended work; it isn't for indicating that you've worked on something, that's for the maintainers to identify via the comments and issue credits system. Thanks everyone!

And devel.info.yml itself is missing it too.

Also updated the javascript test to use the web driver test base class as that was breaking test discovery on D9. With this applied, I can at least run the tests. With that, got a lot of these:

31) Drupal\Tests\webprofiler\Kernel\DecoratorTest::testEntityTypeDecorator with data set #4 ('state', 'Drupal\Core\State\State', 'Drupal\webprofiler\State\StateWrapper')
PHPUnit\Framework\Exception: PHP Fatal error: Declaration of Drupal\webprofiler\Profiler\DatabaseProfilerStorage::find($ip, $url, $limit, $method, $start = NULL, $end = NULL) must be compatible with Symfony\Component\HttpKernel\Profiler\ProfilerStorageInterface::find($ip, $url, $limit, $method, $start = NULL, $end = NULL): array in /home/berdir/Projekte/d8/modules/devel/webprofiler/src/Profiler/DatabaseProfilerStorage.php on line 12

Fixed that by adding the return type definitions to those methods. Problem is that this requires PHP 7.1, Drupal 8.8 however only requires 7.0, so I added that to webprofiler.info.yml. You won't get around that if you want to be D9 compatible.

There's more that shows that you will need to require 8.8, e.g. for devel_generate, the alias manager service access in \Drupal\devel_generate\Plugin\DevelGenerate\ContentDevelGenerate::create. That's the kind of stuff that only running tests with enabled deprecation messages (or against D9) will show.

Posting this as a WIP atch.

I see there's a dedicated issue for the test, so removing that again from this. Also saw an issue for the alias manager. Downside of keeping things in very small issues is that you're going to get a bit of an overlap with the .info.yml version requirement. But you could get this in and then update it over there.

Thanks @DamienMcKenna and @Berdir, this is helpful.

That's the kind of stuff that only running tests with enabled deprecation messages (or against D9) will show.

Yes, absolutely. That's why I run all Devel tests on Github/Travis to show all deprecation messages. We also have 3042575-11.show_deprecations.patch on #3042575: [META] Deprecation issues in devel module

According to #3096609: Allow contrib test modules to not need a core or core_version_requirement key and alexpott's comment the test modules do not need the core_version_requirement but they can run with it. Not sure which is best here.

They only don't need it if you have at least drupal core 8.8.2 or so, we just had a problem with page_manager breaking metatag's 8.7 tests becaue drupal blows up in earlier versions without any compatibility key. And metatag actually also had a require-dev dependency on devel that it temporarily removed until this is resolved, that's why I ended up here :) I'd suggest to keep it for now. A change is required anyway as you would then also need to remove the core: 8.x key. And you won't have to touch them again until preparing for D10.

Thanks @Berdir.

From #9
Declaration of Drupal\webprofiler\Profiler\DatabaseProfilerStorage::find ... must be compatible with Symfony\Component\HttpKernel\Profiler\ProfilerStorageInterface::find

Fixed that by adding the return type definitions to those methods. Problem is that this requires PHP 7.1, Drupal 8.8 however only requires 7.0, so I added that to webprofiler.info.yml. You won't get around that if you want to be D9 compatible.

I think it sounds reasonable for Webprofiler to need PHP7.1. This is a developers module after all, and devs are likely to have 7.1+. Nice that the main Devel and Devel_generate can still run on PHP7.0 for now.

There's more that shows that you will need to require 8.8, e.g. for devel_generate, the alias manager service

Yes. I also think that we'd be OK to have Devel 8.x-3.x only work from core 8.8+ for the same reasons. This is a developers module, we have the 2.x branch for core 8.7, and as you say on #3095363-9: Replace deprecated \Drupal\Core\Path\AliasStorage Devel 8.x-3.x is does not yet have a stable release (this is also being discussed on #3112287: Plan for Devel 8.x-3.0 / 4.0.0 release)

Respondong to #13 OK, we'll leave in the addition of core_version_requirement: ^8 || ^9 for the four test modules.

So are you saying that patch 10 (in comment #11) is what you need to allow installing at D9 and to run the tests? If so we should look to commit it fairly promptly.

> So are you saying that patch 10 (in comment #11) is what you need to allow installing at D9 and to run the tests? If so we should look to commit it fairly promptly.

Kinda. It will allow DrupalCI to "composer require" the module into a D9 site, but it will fail hard with a class not found error during test discovery due to #3097125: Replace deprecated JavascriptTestBase with WebDriverTestBase as that base class is gone on D9. But then we can then run that patch over there against D9 and hopefully get to the next step, with specific test fails in some tests. At least that's what I saw locally.

So if we did limit the Devel 3.x branch to core 8.8+ then the patch needs to have core_version_requirement: ^8.8 || ^9. Also all of the core: 8.xhave to be removed.

I expected the 8.7 test to fail right at the installation phase. But the main module did get installed and the tests started. I think this is because the devel.info.yml file is not re-evaluated after the patch. But when the tests started, the test modules could not be installed because they had ^8.8 || ^9

For 9.x the composer requirements are analyzed before the patch is applied. So we can't test using core 9.0 on drupal.org until the .info.yml file changes are committed.

Correct, the composer require is based on the available information on drupal.org.

Thanks @Berdir, good to have my deductions confirmed.

As I am a relatively recent co-maintainer for Devel I checked with @moshe weitzman and he has given the OK on #3095587-9: What Core, PHP and Drush versions does Devel support / require? for the 8.x-3.x branch to drop core 8.7 support and for webprofiler to require PHP7.1

@Berdir, I have committed patch #16 apart from the single line change to webprofiler/src/DataCollector/PhpConfigDataCollector.php which originated in your patch #9

-  use StringTranslationTrait, DrupalDataCollectorTrait, LinkGeneratorTrait;
+  use StringTranslationTrait, DrupalDataCollectorTrait;

Can you tell us what led you to make this change? I know that our test coverage for Webprofiler is not very comprehensive, so an error here would not necessarily show as a test failure. If it really does not need LinkGeneratorTrait then I will also delete the 'use' statement at line 5. Thanks for your help on this.

LinkGeneratorTrait does not exist in Drupal 9 :)

I'm not sure why it was there, didn't see anything that needed it. I suspect that you refactored that away somewhere else because you did get deprecation messages from actually using it but forgot to remove the trait.

And yeah, for the same reason I forgot to remove the use statement :)

Branch test at 9.0 fails with symfony/var-dumper requirement which needs 5.0. Also Travis tests need to allow for drush 9 or 10.

I have now added a 3.x branch test on commit at Core 9.x

General 9.x infrastrucure changes (drupalci.yml, composer.json, .travis.yml, etc) can be discussed and tested here on this issue.

Patches can now be tested against 9.x but currently fail (as expected) for the known reasons on #3042575: [META] Deprecation issues in devel module. Fixes to specific deprecated code should be done on those child issues, not here.

Marking this issue as fixed, as there is nothing remaining to do right here, but that may well need to be changed if/when we discover new things.

Thank you @Berdir @vipin.mittal18 and @DamienMcKenna for your help. This is a nice step forward for Devel.

Excellent, great work everyone, and thank you jonathan1055 for stepping up to comaintain the project!

Looks like the commit on #23 took care of the following child issue that was pending for review for weeks.
Remove the deprecated Drupal\Core\Routing\LinkGeneratorTrait

I actually, spent lots of time on those issues and thought we were suppose to fix them on child issue to catch errors and make it easier for commits.
There might be others as well (didn't check them all.)

git log -p webprofiler/src/DataCollector/PhpConfigDataCollector.php
commit 3365c7545c337209757a80ccd8710cfae479c645
Author: berdir <berdir@214652.no-reply.drupal.org>
Date:   Thu Apr 2 13:47:15 2020 +0100

    Issue #3118857 by Berdir, jonathan1055: Remove unused LinkGeneratorTrait from Webprofiler Ph

diff --git a/webprofiler/src/DataCollector/PhpConfigDataCollector.php b/webprofiler/src/DataColl
index 9ce4e61..461c8e9 100644
--- a/webprofiler/src/DataCollector/PhpConfigDataCollector.php
+++ b/webprofiler/src/DataCollector/PhpConfigDataCollector.php
@@ -2,7 +2,6 @@
 
 namespace Drupal\webprofiler\DataCollector;
 
-use Drupal\Core\Routing\LinkGeneratorTrait;
 use Drupal\Core\StringTranslation\StringTranslationTrait;
 use Drupal\webprofiler\DrupalDataCollectorInterface;
 use Symfony\Component\HttpKernel\DataCollector\DataCollector;
@@ -14,7 +13,7 @@ use Symfony\Component\HttpFoundation\Response;
  */
 class PhpConfigDataCollector extends DataCollector implements DrupalDataCollectorInterface {
 
-  use StringTranslationTrait, DrupalDataCollectorTrait, LinkGeneratorTrait;
+  use StringTranslationTrait, DrupalDataCollectorTrait;

Looks like the commit on #23 took care of .. #3118260: Remove the deprecated Drupal\Core\Routing\LinkGeneratorTrait

... thought we were suppose to fix them on child issue

@salah1, you are right on both accounts. I'm sorry, that was my fault, I should have noticed that the change was already on another issue, I will credit you on that issue when I mark it fixed. That is the only one, the rest are still to be fixed on the child issues of #3042575: [META] Deprecation issues in devel module not here.

Fix deprecated function file_save_htaccess()
Please review. Thanks

Thanks Farnoosh but we already have this on #3100292: Replace deprecated file_save_htaccess() with \FileSecurity::writeHtaccess()

If you'd like to work on D9 issues see #3124519: Class 'Doctrine\Common\Util\Debug' not found when running Drupal 9.x and #3042575: [META] Deprecation issues in devel module

The commit in #33 is to allow the D9 branch and patch testing to run without

run-tests.sh fatal error
Uncaught Error: Class 'Drupal\FunctionalJavascriptTests\JavascriptTestBase' not found

which prevents all other useful test output. For more details see #3097125-22: Replace deprecated JavascriptTestBase with WebDriverTestBase commenst #22 - #31

Raised #3126029: Quoted query text in DevelQueryDebugTest - D9

I have fixed #3126029: Quoted query text in DevelQueryDebugTest - D9

A new problem #3127750: Using assertContains() with string haystacks is deprecated and will not be supported in PHPUnit 9 has just started being reported in the 9.x tests.

With #3097125: Replace deprecated JavascriptTestBase with WebDriverTestBase and #3127750: Using assertContains() with string haystacks is deprecated and will not be supported in PHPUnit 9 both fixed and we now have no deprecaton warnings when running tests at 9.0.x

Removing outdated tags meant for core issues.

This issue has been migratated to https://gitlab.com/drupalspoons/devel/-/issues/224

There's a 4.x branch that proclaims drupal 9 compatibility;
- is 4.x compatible with D9
- if so, should we close this issue?

Yes the 4.x branch is compatible with Drupal 9. Version 4.0.1 has just been relesed.
We should let @moshe have the final say, but I think this issue can be marked 'fixed'?

I've installed the latest version (4.1) of Devel which is considered to be compatible with Drupal 9, I've detected some deprecated code into the module code: used of entity.manager instead of entity_type.manager service.

Also when I've installed the latest version of search_api module which is compatible with Drupal 9, I've got the following error message from webprofile devel submodule while trying to create a new media image:

TypeError: Argument 1 passed to Drupal\search_api\Plugin\search_api\datasource\ContentEntityTrackingManager::__construct() must be an instance of Drupal\Core\Entity\EntityTypeManager, instance of Drupal\webprofiler\Entity\EntityManagerWrapper given, called in /Users/b.khouy/Sites/my_project/core/lib/Drupal/Component/DependencyInjection/Container.php on line 259 in Drupal\search_api\Plugin\search_api\datasource\ContentEntityTrackingManager->__construct() (line 57 of /Users/b.khouy/Sites/my_project/modules/contrib/search_api/src/Plugin/search_api/datasource/ContentEntityTrackingManager.php)

Hello b.khouy,
Development and Issue tracking for Devel has moved to https://gitlab.com/drupalspoons/devel so please raise an issue on drupalspoons
More info is on the Devel project page