Problem/Motivation

Steps to reproduce

Proposed resolution

Use upgrade_status module to identify changes. Make this module D10 compatible. Create patch.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#12 3363000-12.patch2.12 KBrsnyd
#10 3363000-10.patch1.79 KBrsnyd
#8 3363000-8.patch1.39 KBjsacksick
#7 3363000-7.patch1.28 KBrsnyd
#2 3363000-2.patch958 bytesrsnyd

Issue fork commerce_shipstation-3363000

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

rsnyd@yahoo.com created an issue. See original summary.

rsnyd’s picture

rsnyd
Primary language English
Location Northeast Pennsylvania
commented
StatusFileSize
new 3363000-2.patch958 bytes

c_archer opened merge request !6

c_archer’s picture

c_archer commented
Status: Active » Needs review
jsacksick’s picture

jsacksick commented
Status: Needs review » Needs work

So, this patch is not really correct as it should also drop D8 compatibility.

Symfony\Contracts\EventDispatcher\Event; doesn't exist on D8 since it's provided by the symfony/event-dispatcher-contracts package with is a dependency starting Drupal 9. So this patch should drop D8 compatibility.
After this, #3343319: Update core_version_requirement in .info file. can probably marked as a duplicate.

Also you might want instead to use the bridge event class provided by Drupal core (See https://www.drupal.org/node/3159012 for explanations).

jsacksick’s picture

jsacksick commented

Also, I'm pretty sure the tests themselves have to be updated to run on D10.

rsnyd’s picture

rsnyd
Primary language English
Location Northeast Pennsylvania
commented
StatusFileSize
new 3363000-7.patch1.28 KB

@jsacksick,
Thank you for the helpful feedback. So I've changed the .info.yml file to drop the D8 compatability. ShipStationOrderExportedEvent now uses Drupal\Component\EventDispatcher\Event.

These two changes are included in the patch.

As for the tests, there is really only one that ShipStationKernelTestBase is the only class that contains real tests, i.e. not just stubs/function signatures. This test only references commerce_order, commerce_shipping, commerce_product and profile packages, so I don't think there's anything to do to get it to run on D10. The root composer.json, however, requires "drupal/commerce": "^2.17" and "drupal/commerce_shipping": "^2.1". Do these versions need to be updated to "drupal/commerce": "^2.36" and/or "drupal/commerce_shipping": "^2.6"?

Thank you for the help.

jsacksick’s picture

jsacksick commented
Status: Needs work » Needs review
StatusFileSize
new 3363000-8.patch1.39 KB

Oh... I see the tests are just useless (just checked the code), there's an additional change required to properly support D10. In the link I sent, it is explained that the order of parameters changed when calling the dispatch() method on the event dispatcher.

Also, the bridge class has been introduced in Drupal core 9.1 (it is technically not there in 9.0).

Note that the patch you sent isn't applicable as is (it shouldn't be generated from the drupal root but you rather need to clone the module's repo, make your changes and then use the git diff command (See https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa...).

Also had a quick look at the module in general and as far as I can see there are several coding standard issues, fixing these is probably not in the scope of this issue though... There's code commented out, calls to \Drupal:: from within services rather than using dependency injection (e.g: \Drupal::moduleHandler()->alter('commerce_shipstation_order_xml') from ShipStation.php rather than $this->moduleHandler

c_archer’s picture

c_archer commented

Lets create a ticket for the coding standards and I'm happy to pick up.

rsnyd’s picture

rsnyd
Primary language English
Location Northeast Pennsylvania
commented
StatusFileSize
new 3363000-10.patch1.79 KB

@jsacksick,
Ok, my changes include the following:

  • core_version_requirement in info.yml to be ^9.1 || ^10
  • use statement in ShipStationOrderExportedEvent to use bridge class
  • Type hints in ShipStation to use \Symfony\Contracts\EventDispatcher\EventDispatcherInterface (from your link, NOTE: I did not change the use statement to match since the link only specifically mentions the type hints and "Since Symfony 4.3, Symfony\Component\EventDispatcher\EventDispatcherInterface extends Symfony\Contracts\EventDispatcher\EventDispatcherInterface")
  • I have created the patch from the commerce_shipstation cloned repository folder this time
  • I have not touched the coding standards or the addressed the dependency injection issues since I'm just trying to get my first patch accepted and I figure simpler is better. Happy to address these in another issue, but I see that @c_archer has already volunteered

I hope this one is correct. Thank you for taking the time to help me.

jsacksick’s picture

jsacksick commented

The event dispatcher parameter change isn't present in your patch (see my patch), this breaks in D10.

rsnyd’s picture

rsnyd
Primary language English
Location Northeast Pennsylvania
commented
StatusFileSize
new 3363000-12.patch2.12 KB

@jsacksick,
So sorry, I'm not sure how that happened.

Thank you again.

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

rwanth opened merge request !7

  • rwanth committed 4aa8dbf0 on 2.x 
    Issue #3363000 by rsnyd, jsacksick, c_archer: Drupal 10 Compatibility
rwanth’s picture

rwanth
Primary language English
Location New York, USA
commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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

c_archer’s picture

c_archer commented
c_archer’s picture

c_archer commented