Problem/Motivation

Commerce 3.3 updated it's OrderTotal ViewsArea class.
OrderItemOrderTotal now throws a fatal error since it extends Commerce's OrderTotal.

Fatal error: Type of Drupal\commerce_add_to_cart_confirmation\Plugin\views\area\OrderItemOrderTotal::$entityTypeManager must not be defined (as in class Drupal\commerce_order\Plugin\views\area\OrderTotal) in /var/www/html/web/modules/contrib/commerce_add_to_cart_confirmation/src/Plugin/views/area/OrderItemOrderTotal.php on line 20

It looks like OrderItemOrderTotal isn't needed anymore.

Steps to reproduce

Install Commerce 3.3 and this module.

Proposed resolution

This feels like a change that'd require cutting a new module version requiring commerce_order >=3.3 and completely removing OrderItemOrderTotal.

CommentFileSizeAuthor
#9 3578120-support-commerce-3.3-no-class-deletion-8.patch1.46 KBgrevil

Issue fork commerce_add_to_cart_confirmation-3578120

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

johnny5th created an issue. See original summary.

johnny5th changed the visibility of the branch 1.x to hidden.

johnny5th opened merge request !24

johnny5th’s picture

johnny5th commented
Status: Active » Needs review

It doesn't look like anything in the codebase uses OrderItemOrderTotal. I pushed a draft MR to remove the plugin.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Title: Support Commerce 3.3 - Fatal Error » Commerce 3.3 - Fatal Error
Version: 2.0.0 » 2.x-dev
Priority: Normal » Critical
Status: Needs review » Needs work

Same issue here! I can confirm this.

grevil’s picture

grevil commented

@johnny5th can you explain in more detail why we should remove the entire "OrderItemOrderTotal" class?

The issue you describe only happens, because both the "OrderItemOrderTotal" class itself and its parent define an "entityTypeManager" instance. And at first glance, both classes define entirely different render methods.

johnny5th’s picture

johnny5th commented

Apologies for not including those details. It appears that the Commerce 3.3 updated their OrderTotal class to fulfill the needs that OrderItemOrderTotal class was filling (passing in the order item ID). https://git.drupalcode.org/project/commerce/-/commit/532d3ad0351d56949e6...

But you're right, it looks like the core render method uses the commerce_order_total_summary theme instead of the default. I probably just tried it and it worked well so I (potentially wrongly) assumed it was better to use Core's native class output.

grevil opened merge request !25

grevil’s picture

grevil commented
Status: Needs work » Needs review
StatusFileSize
new 3578120-support-commerce-3.3-no-class-deletion-8.patch1.46 KB

Ah I see! Thanks for the clarification! :)

Let's keep both MRs open and let @tomtech decide here!

I attached MR 25 as a patch for the time being.

grevil’s picture

grevil commented

@tomtech, could you also set 2.x as the default branch for future devlopment? Thanks! 👍

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs review » Reviewed & tested by the community

MR!25 fixes the issue and looks correct to me.

Seems this commit crashed it: https://git.drupalcode.org/project/commerce_add_to_cart_confirmation/-/c...

As a result I'd vote for better test coverage in the module. @tomtech please see #10.

This is critical, so a new tagged release would be great or it will break further projects upgrading to Commerce 3.3!

tomtech’s picture

tomtech commented
Assigned: Unassigned » tomtech
Status: Reviewed & tested by the community » Needs work

Hi all,

Thanks for the report and info.

First, IRT Comment #10, default branch has been updated. Thanks for pointing that out.

IRT the issue, this occurred because of the change in the parent class now declaring $entityTypeManager, which it didn't previously. We really should have strongly typed that property declaration, as that would have avoided this issue, and is the more appropriate declaration.

If we went with MR25, it would break any sites NOT running Commerce 3.3+, so that isn't really an option.

This class has been around a while, but it's not really clear why it was subclassed from OrderTotal, as it isn't reusing anything from there except for the description that is added in buildOptionsForm().

As @johnny5th noted, Commerce 3.3 potentially makes this plugin obsolete, but removing it all together would break sites that haven't yet upgraded to 3.3.

I'm going to make the fix by just subclassing `AreaPluginBase`, rather than `OrderTotal`. (And, I guess, copying the buildOptionsForm override) This will avoid the conflict in 3.3, still work with Commerce versions prior to 3.3.

tomtech changed the visibility of the branch 3578120- to hidden.

tomtech opened merge request !26

  • b8822e12 committed on 2.x 
    feat: #3578120 Commerce 3.3 - Fatal Error

By: johnny5th
By: anybody
By...
tomtech’s picture

tomtech commented
Status: Needs work » Fixed

Fix merged. Can I get a confirmation from one of y'all before I tag a new release?

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

grevil’s picture

grevil commented

Thanks @tomtech! Yea you are right, I forgot to adjust the version requirement to commerce >= 3.3, but you're approach is definitly better for backwards compatibility!

I can confirm, that the error is gone in current 2.x-dev and that the add to cart confirmation still works as expected! RTBC!

Status: Fixed » Closed (fixed)

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