Problem/Motivation

Currently this module is Incompatible with Drupal 10

Proposed resolution

Use drupal-rector to create a patch to make it compatible

CommentFileSizeAuthor
#6 commerce_price_rule-d10compatibility-3404466-6.patch3.91 KBestebanvalerio.h
#2 commerce_price_rule-d10compatibility-3404466-2.patch2.16 KBestebanvalerio.h

Issue fork commerce_price_rule-3404466

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

estebanvalerio.h created an issue. See original summary.

estebanvalerio.h’s picture

estebanvalerio.h commented
StatusFileSize
new commerce_price_rule-d10compatibility-3404466-2.patch2.16 KB

I created a patch using drupal-rector and grabbing the files it modified, after flushing cache and using Upgrade Status, it recognizes the module as a compatible D10 one.

sarwan_verma made their first commit to this issue’s fork.

sarwan_verma opened merge request !5

sarwan_verma’s picture

sarwan_verma commented
Status: Active » Needs review
estebanvalerio.h’s picture

estebanvalerio.h commented
StatusFileSize
new commerce_price_rule-d10compatibility-3404466-6.patch3.91 KB

While testing the project where I'm using this module I found an error:

Drupal\Core\Entity\Query\QueryException: Entity queries must explicitly set whether the query should be access checked or not. See Drupal\Core\Entity\Query\QueryInterface::accessCheck(). in Drupal\Core\Entity\Query\Sql\Query->prepare() (line 141 of /app/public_html/core/lib/Drupal/Core/Entity/Query/Sql/Query.php).

I noticed in this module, some entity queries are run and needed that line, so I create a new patch for it.

alexpott made their first commit to this issue’s fork.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Reviewed & tested by the community

I've merged the fixes in #6 with MR and removed Drupal 8 compatibility as Drupal 8 is long unsupported.

krystalcode made their first commit to this issue’s fork.

  • krystalcode committed 78bfacfd on 8.x-1.x 
    Issue #3404466 Removed multiple calls to access check

Already called...

  • krystalcode committed 6c53b60c on 8.x-1.x 
    Issue #3404466 Corrected method declaration to match interface

krystalcode’s picture

krystalcode commented

Merged into the main development branch. I will close and make a release after this has been tested on a project for some time to make sure we don't have any regressions.

Notes:
- I changed accessCheck() to accessCheck(TRUE); I prefer to be explicit with access control, it makes it easier to read if you don't remember what is the default.
- Removed a duplicate call to access check, kept the one before the subscriber so that subscribers can change it if they have a legitimate reason.
- I reverted the change in the declaration of public static function getSubscribedEvents() to match that of the interface - see Symfony\Component\EventDispatcher\EventSubscriberInterface.
- Good with removing Drupal 8 support, event dispatching is not compatible anymore.

Thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@krystalcode return typehints can be more explicit than the interface. Reverting that just means you'll have to do it for Drupal 11 compatibility.

krystalcode’s picture

krystalcode commented

@alexpott why would that be the case? Drupal 11 relies on Symfony 7 (https://git.drupalcode.org/project/drupal/-/blob/11.x/core/composer.json...) which also has the same method declaration (https://github.com/symfony/event-dispatcher/blob/7.1/EventSubscriberInte...).

krystalcode’s picture

krystalcode commented

Updated credits.

pfrenssen’s picture

pfrenssen
Location Sofia
commented
Status: Reviewed & tested by the community » Fixed

This has been committed a long time ago, and there have been no reports of any problems, so I guess it is OK to mark this as fixed. Thanks all!

Status: Fixed » Closed (fixed)

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