Automated Drupal 11 compatibility fixes for feeds

Update bot seems to also miss the following errors:

Fatal error: Declaration of Drupal\feeds\Entity\Feed::__wakeup() must be compatible with Drupal\Core\Entity\EntityBase::__wakeup(): void in /var/www/html/web/modules/contrib/feeds/src/Entity/Feed.php on line 97

Fatal error: Declaration of Drupal\feeds\Entity\FeedType::__sleep() must be compatible with Drupal\Core\Config\Entity\ConfigEntityBase::__sleep(): array in /var/www/html/web/modules/contrib/feeds/src/Entity/FeedType.php on line 210

Will work as per @ptmkenny suggestion and pushed changes required for D11 only rest phpcs and phpstan fixes will be covered separately.

Please use DeprecationHelper in that system_time_zones() call.

The watchdog_exception can simply be changed to use Error::logException directly, as that exists since Drupal 10.1 (see https://www.drupal.org/node/2932520). Or you can use DeprecationHelper here if we want to keep compatibility with Drupal <10.1.

Don't add drush.services.yml for Drush 13. Drush 12 has deprecated this and it will be removed in 13 (see https://www.drush.org./12.x/commands/ and https://www.drush.org./13.x/commands/).

This also needs to handle the problem I reported in #6.

@jcnventura: As this module currently using 9,10 and 11 so I guess we can't use deprecation helper as this was introduced in 10.1.x and it is helpful when you are checking D10.1 and above please check https://www.drupal.org/node/3379306

Now it totally depends upon maintainer to support of Drupal versions only D10 and D11 only then I'll use deprecation helper otherwise version_compare or class_exists is better way to go ahead.
For drush deprecation: this module is still supporting previous versions of drush so I also let this decision on maintainer how they think to go ahead.
cc: @megachriz

@ankitv18
I'd like to create one last Drupal 9 compatible release of Feeds (hopefully next week, I'm already busy writing the release notes and see what I want to commit last minute) and then after that Drupal 9 support can be dropped so focus can be on getting Feeds compatible with Drupal 11.

I haven't paid attention to the Drush integration, but I think we only would need to support Drush 12 and above? Or maybe Drush 11 too? At least we should be compatible with the latest Drush.

You can see Drush compatibility here: https://www.drush.org./13.x/install/#drupal-compatibility

If you want to drop support for Drupal 9, you can choose to also only support Drush 12 and above. This would simplify the Drush support, as you can then simply drop the drush.services.yml file and move the Drush commands class file to /src/Drush/Commands. See https://www.drush.org./13.x/commands/

Alright, then only support Drush 12 and above it is. I'll leave that to somebody else to get that done (at least for now), so I can focus on getting the last bits done for the upcoming release.

+1 on @jcnventura comment#18
And it would ease to maintain the Drupal version, furthermore neither we need to use deprecationHelper or any conditional statement for BC

And yet, this line is in the MR:
"drush.services.yml": "^9 || ^10 || ^11 | ^12 || ^13"

Drush 13 will not support the drush.services.yml file (Drush 12 deprecated it). This shouldn't be merged like this.

Also this line:

class_exists(Error::class) ? Error::logException(\Drupal::logger('feeds'), $e) : watchdog_exception('feeds', $e);

This is effectively removing support for Drupal <10.1. Class Error exists since Drupal 8. The logException method was added in Drupal 10.1. This code will always go in the true section of the ternary op, and throw a fatal error if it is not Drupal >=10.1.

Considering @jcnventura point at #24 of Drush 13 support, It be would be done in separate issue i.e https://www.drupal.org/project/feeds/issues/3453031 (already linked to this issue) and for log exception to support BC I've used method_exists and logException method is introduced in D10.1.

cc: @MegaChriz

Rebased MR !173 with 8.x-3.x branch. Verified changes locally and looks good to me.

There are few deprecation which should be fixed as part of this ticket

Instantiation of deprecated class                                      
         Drupal\Core\Password\PhpassHashedPassword:                             
         in drupal:10.1.0 and is removed from drupal:11.0.0. The                
           password compatibility service has been moved to the phpass module.  
           Use \Drupal\phpass\Password\PhpassHashedPassword instead. 

There's already a separate issue where all the phpstan fixes are considered. Please check below issue
https://www.drupal.org/project/feeds/issues/3425218

@MegaChriz I've noticed you pushed logException Method few days back: https://git.drupalcode.org/project/feeds/-/merge_requests/127/diffs#a56d...
And going through the feeds.services.yml below logger is already there.

  logger.channel.feeds:
    parent: logger.channel_base
    arguments: ['feeds']

In this MR I used direct drupal call using \Drupal::logger('feeds')->error($e) instead of watchdog_exception
For PhpassHashedPassword deprecation I've replaced it with a https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Password%...

Rest of the PhpStan warnings can be covered in https://www.drupal.org/project/feeds/issues/3425218

Changes looks good now for D11 compatibility.

It looks like that some new phpcs issues are introduced with the code changes. We could consider to resolve #3425218: Fix PHPStan errors first, to also possible catch newly added phpstan issues.

Anyway, needs work. I think that at least tests must pass on Drupal 11 before merging it. Else there would be a risk to release something as D11 compatible while it may still have some incompatibility.

@MegaChriz when you merge this, it might make sense to create a 4.x branch, and to tag a 4.0.0-alpha1 version when you add D11 support. This would allow you to keep adding changes to the 8.x-3.x branch in the case that something critical should happen, and to pivot Drupal 10 and 11 to fully use semantic versions.

Updated the MR and enabled the next major pipeline, please review the MR!173
Created new issue to support book and laminas-feed tests and fix all deprecated method introduced in phpunit 10
https://www.drupal.org/project/feeds/issues/3454788

cc: @MegaChriz

@MegaChriz when you merge this, it might make sense to create a 4.x branch, and to tag a 4.0.0-alpha1 version when you add D11 support. This would allow you to keep adding changes to the 8.x-3.x branch in the case that something critical should happen, and to pivot Drupal 10 and 11 to fully use semantic versions.

@jcventura
Yeah, I would love to get semantic versioning for this reason, but a few people strongly adviced me to not create a new major version because that would slow down adoption of the new version + it creates an extra hurdle for people who want to upgrade to a new major Drupal version, Drupal 11 in this case.

For details, see https://drupal.slack.com/archives/C014CT1CN1M/p1659448352144959
See also https://medium.com/jakob-on-drupal/dont-go-making-major-version-changes-...

Anyway, I'm on holiday soon, so I'll probably not be able to commit this shortly - unless I've got so much bad weather on location that I just want to work on Feeds.

@MegaChriz, your module and I do agree with not bumping version numbers needlessly. However, if you're introducing breaking changes, a major bump is justified. And for me, dropping support for Drupal 9 is one such BC break (assuming you're still going to do that as per #16).

Even if the MR is not yet dropping Drupal 9.3, it is a possible easy solution for my comments on the MR. The other possible solution (adding PHP 8 as a minimum) is also a BC-break.

Looking at the update bot suggestion, I think we need to use TimeZoneFormHelper::getOptionsList() as we actually want to see the timezone labels in the form, and not only use the timezone identifiers.

Also, as per #16, let's make this simple and drop support for Drupal 9, and only support ^10.1 || ^11.

Thanks @jcnventura,
I have dropped the D9 and used timeZoneHelper as you suggested.
Please review the MR and make it ease for @MegaChriz

I've added some remarks. Tests don't pass yet on Drupal 11. I would say that is the minimum requirement for merging this MR. I think that temporary skipping the book tests to get tests pass on Drupal 11 is acceptable, but I think that doesn't count for tests/src/Unit/Feeds/Parser/SyndicationParserTest.php, because the syndication parser is in Feeds itself while the Book target requires an additional module.

There's overlapping with the work that is being done in #3425218: Fix PHPStan errors. I think it would simplify the work here if that issue is done first. The downside is that it perhaps takes a bit longer to get Feeds Drupal 11 compatible. I'm trying my best though to get #3425218: Fix PHPStan errors done quickly. My goal is to get Feeds compatible with Drupal 11 at least in dev before July 29. For a new release I'd like to also get #3453031: Support Drush 12 and above only resolved and having all checks passed on GitLab - if possible.

Thanks @MegaChriz,
I'll do the changes as per the feedback once phpstan fixes are fixed: https://www.drupal.org/project/feeds/issues/3425218

I see that there are the following test failures on D11:

• views.view.feeds_import_logs:display.default.display_options.arguments.feed.default_argument_skip_url
  missing schema
• Drupal\Tests\feeds\Functional\Feeds\Fetcher\UploadFetcherTest::testImportSingleFile: Error: Call to a member function getItemCount() on null
• Drupal\Tests\feeds\Functional\Feeds\Parser\Form\CsvParserFeedFormTest: Error: Call to a member function getItemCount() on null
• Drupal\Tests\feeds\Functional\Update\
  Exception: No suitable core fixture found.

I also see in the PHPStan report that there is one watchdog_exception() call left, namely in src/FeedsQueueExecutable.php.

So next steps are:

1. Figure out what views.view.feeds_import_logs:display.default.display_options.arguments.feed.default_argument_skip_url is, maybe the Views configuration code for the import logs needs to be regenerated?
2. Tests in Drupal\Tests\feeds\Functional\Feeds\Fetcher\UploadFetcherTest are failing. Figure out why.
3. Tests in Drupal\Tests\feeds\Functional\Feeds\Parser\Form\CsvParserFeedFormTest are failing. Figure out why.
4. Tests in Drupal\Tests\feeds\Functional\Update, used to test update functions in feeds.install and feeds.post_update.php are failing, probably because it is trying to use a fixture (aka database dump) that is no longer available in Drupal 11. Solution: try to use a different provided fixture.
5. Resolve watchdog_exception() call in src/FeedsQueueExecutable.php.

Of the test failures, I plan to look first at the Update tests. I've done these before, so I have a good idea how to handle these. If I remember correctly, I've written code to select a fixture based on the current core version. Hopefully I can do these tomorrow or else Saturday.

@ankitv18
If you have time to help on this issue in the next few days, could you look at the failures related to Views first?

I've updated the remaining tasks in the issue summary.

Hopefully we can resolve the test failures quickly, only 1 to 2 weeks before Drupal 11.0 is released and it would be nice to have Drupal 11 supported in at least the dev version of Feeds by then. It's probably to late to have it in an actual release as well. When this issue gets resolved, I'd like to have at least two weeks testing the changes on a few live sites before making a release. And preferable including D11 compatible code of Feeds Extensible Parsers and Feeds Tamper too, which we cannot really start on yet before this is resolved.

I fixed the Update tests and a few other tests too. I plan to continue on Saturday. Feel free to work on this issue until that time.

Remaining tests to fix:

1. Fix test Drupal.Tests.feeds.Kernel.RevisionableEntityTest testWithMappingToRevisionFields
2. Fix test Drupal.Tests.feeds.Kernel.Entity.FeedTest testDispatchImportFinishedEvent
3. Fix test Drupal.Tests.feeds.Kernel.Feeds.Target.UserRoleTest testImportWithExistingRole

See also https://git.drupalcode.org/project/feeds/-/pipelines/227854/test_report?...
(not sure if the link remains valid when committing new code.)

Tests are passing on D11! I think this is ready for review and testing.

Note that phpstan (next major) is failing, but I think fixing that is out of scope for this issue.

I've looked through all the changes and made a few minor changes a few hours ago. The code prior to these last changes have been running 2 to 3 days on four D10 sites and I haven't catched any regressions yet.

So I'm merging this now!

Not creating a new release yet, because I think 3 days is too short for catching regressions. Also, I like to see #3453031: Support Drush 12 and above only and #3454788: PHPunit Next Major pipeline failure to be resolved before the next release. And it would be good to test Feeds manually on a D11 site.

But at least by merging this, modules that extend Feeds can be made compatible with D11 too.

Thanks all!

The issue is left open to allow the update bot to post new fixes, should there be any.

@megachriz Saw D11 compatible RC release is out there ~~ now we can close this one?

@ankitv18
I'm not sure, but I think we should best leave it open. Theoritically, there could be new D11 compatibility patches, as said in the issue summary:

Leaving this issue open, even after committing the current patch or merging the MR, will allow the Project Update Bot to post additional Drupal 11 compatibility fixes as they become available in Drupal Rector.

Hi @megachriz,
There are no new patches by projectBot so I guess we can mark this one fixed