Fix PHPStan errors

Some work has been done, but it is not complete yet.

I updated the branch with latest 3.x but the PHPUnit script is still failing, not sure why.

For fixing PHPStan issues related to State and CleanState I opened the spin-off issue 📌 Add StateFactory for fixing a PHPStan issue Active .

I'm ignoring the warning about the deprecated interfaces from laminas/laminas-feed, because there isn't a way provided yet to not use these interfaces. See https://github.com/laminas/laminas-feed/issues/85.

Fixing PHPStan issues in Feeds is tough. I’m not sure if I’ll be able to fix them all. Trying to fix the ones in FieldTargetBase might cause problems for classes extending FieldTargetBase, so perhaps I don’t fix these.

It looks like @franz is no longer working on this issue and since I'm working on it for some days now, I assign it to myself.

Hi @MegaChriz
Could you please review this issue https://www.drupal.org/project/feeds/issues/3430449 📌 Automated Drupal 11 compatibility fixes for feeds Needs review (D11 compatibility) and see whether the changes can be mergeable or not.
Then phpstan issues can be fixed afterward.

@ankitv18
Thanks for the heads up, I'll look at 📌 Automated Drupal 11 compatibility fixes for feeds Needs review first then.

I think that some of the errors reported by PHPStan cannot be fixed without BC breaks. These are

• The fact that \Drupal\Feeds\Plugin\Type\PluginBase::getConfiguration() doesn't always return an array and therefore does not comply to the interface \Drupal\Component\Plugin\ConfigurableInterface. Current code is expecting getConfiguration() to return sometimes something else than an array.
  Opened 📌 (PHPStan) Deprecate calling PluginBase::getConfiguration() with a parameter Active for that.
• \Drupal\Feeds\Plugin\Type\PluginBase doesn't use dependency injection for some services. Adding these to the constructor could break child classes that call the parent's constructor. In this case I think the methods using the non-injected dependencies could be removed, but that would also be a BC break.
  Opened 📌 (PHPStan) Deprecate link and url related methods in PluginBase Active for that.
• \Drupal\Feeds\Plugin\Type\Target\FieldTargetBase doesn't use dependency injection for some services. Just as the one above, adding these to the constructor could break child classes that call the parent's constructor. We could however make the newly added parameters optional and add deprecation warnings when not passed.
  Opened 📌 (PHPStan) Add dependency injection for FieldTargetBase and add deprecation warnings Active for that.
• HttpFetcherResult and RawFetcherResult have an optional parameter for injecting a service. Requiring the parameter would be a BC break. We can however add a deprecation warning when it is not passed.
  Opened 📌 (PHPStan) Add a deprecation warning for constructing a RawFetcherResult without the file_system parameter Active and 📌 (PHPStan) Add a deprecation warning for constructing a HttpFetcherResult without the file_system parameter Active for that.

I created new issues for the above mentioned issues so the changes posted in this issue can be merged earlier, to not hold up 📌 Automated Drupal 11 compatibility fixes for feeds Needs review much longer. I feel that we're running out of time to get Feeds compatible with Drupal 11 before the release of Drupal 11.0.0. Also because of that, I'm planning to merge this in a few days if it passes both PHPStan and PHPUnit. Normally, when there are some larger changes I'd like to apply these on real sites that use Feeds to see if that would reveal any regressions - specifically in combination with modules that extend Feeds. But there is no time for that now to do that properly. I guess I could do that later when Drupal 11 support exists in the dev version.

All these deprecations really make it seem like the version for Drupal 11 should have a new major.

The Drupal 11 MR is now broken, and I don't think it makes sense to fix that before this is committed, so RTBC++.

And please close MR 171 in this issue, so that it doesn't pollute the MR list.

ankitv18 → changed the visibility of the branch 3425218-gitlab-phpstan-issue-fix to hidden.

MegaChriz → closed merge request !171

@jcnventura
I think that adding the deprecation warnings doesn't require a new major version. But at the time that the deprecations get resolved (and the warnings removed), then a new major would be required.
I think releasing a new major right now might not be the right timing, to not create an extra hurdle for people who want to move to Drupal 11. Maybe a 4.x release makes sense in a year from now. This way 8.x-3.x would be compatible with Drupal 10 and 11 and 4.x would require Drupal 11.

I merged the code! I'll now update 📌 Automated Drupal 11 compatibility fixes for feeds Needs review .

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