🇺🇸United States @rhovland

Needs a code review. Update hook has been tested.

Review finished and committed.

Code review done. Committed.

Code is now in branch. Not sure if we want this to actually be committed to the project or not so marking this as postponed.

rhovland → created an issue.

Tested the update hook on a site with 10k reviews. Apparently using hook_update_N(&$sandbox) causes drupal to automatically batch the updates to avoid timeouts.

With that question answered I think this is good to go. Just needs a code review.

Here's a look at the view as installed in a new default Drupal 11 site. It's displaying the rendered entity, so any changes to the template file is reflected here on the individual reviews and field ordering and other stuff is still configured on the entity type display page.

Encountered a problem when creating the view. The product review entity had a data_table definition, but the entity was not translatable.
This was preventing the ability to choose the rendered entity option due to a mismatch in the table name being checked for in Views::fetchPluginNames().
data_table should only be used on translatable entities.

I'm guessing this was a side effect of the product entity code being copied to start development of this module.

I removed the data_table definition from commerce_product_review entity and wrote an update hook to ensure existing data is migrated correctly. There should not be anything to migrate because the data field table was never created by the translation system. This update hook needs to be tested on an install with lots of reviews because I'm uncertain if it needs to be batched since no data is actually updated and it's really just the entity definition being updated (but the update system insists on running a data migration).

rhovland → created an issue.

rhovland → created an issue.

rhovland → created an issue.

Code in 3223947-disable-on-admin now checks if the current theme is set as the site's admin theme and skips adding the libraries if it is. There is no configuration required as it goes off the site's configured admin theme to determine behavior.

Still requires testing to make sure it works.

rhovland → created an issue.

Can confirm this is still an issue on the 2.x branch. However, we're now using Claro as the admin theme the reset is based off of so this won't be an issue for claro users, only those using other admin themes.

The css files are attached in the linked function now so the logic to enable or not needs to go there:
https://git.drupalcode.org/project/media_library_theme_reset/-/blob/2.x/...

Tested again in Drupal 11. Still is broken. Works in Drupal 10.

New install of Drupal 10:

New install of Drupal 11:

While code wise this module is technically compatible with Drupal 11 in practice a lot of critical css is not applying for an unknown reason.

If this old approach (in 1.x branch) to theming the media library in the front-end theme is still important to someone for use in Drupal 11 I welcome a working merge request to make it compatible.

The 1.x branch is not a priority for me so if this is important to you please contribute!

I have decided to move this feature into a moderation submodule since it might be needed or desired by all users of this module.

rhovland → made their first commit to this issue’s fork.

What I was thinking of was implementing natural sorting like what some filesystem browsers implement. It sorts alphabetically but when it encounters numbers it understands what 10 is and sorts as expected. It does not "cast" anything to a number.

So orders with prefixes would still be alphabetically sorted but any numbers within them would be sorted numerically.

I haven't the slightest idea on how this kind of sorting is achieved but I know it's possible because I've seen it done before.

rhovland → created an issue.

Ok I think this is good to go, just need to review the changes again on monday with a fresh set of eyes

Yeah there will never be a field.formatter.commerce_product_review_overall_rating_stars because that's only used for fields added to an entity. And the display settings are not stored there.

They're stored in the core.entity_view_display.{entity_name}.default.default, in this case core.entity_view_display.commerce_product.default.default.yml for a default install. If there are other product types or product view modes there will be additional places to update the configuration.

While it's technically possible to add this overall rating field type to other entities, they will not work because the all of the field code expects there to be a product entity. We probably should add checks for that to prevent broken config forms if admins accidentally add it to other entities in a separate issue. There doesn't seem to be a way to restrict fields to certain entity types.

rhovland → created an issue.

The update hook does not work. The configuration is not changed and the old configuration remains after it is ran.

It's now possible to edit reviews from the administrative screen in the 2.x-dev version but a direct action button to publish and unpublish reviews is needed.

Before:

After:

Tested the latest changes in a drupal-forge test site. Everything functioning correctly.

Here's the admin screen with the view disabled

Here's the admin screen view

I will review the code one more time tomorrow but I think this is probably good to commit.

I decided to revamp the default list builder and move the user page to use that list builder instead, greatly simplifying the code. The administrative page now uses a view by default which allows sorting, searching, filtering and further site builder customization for their workflow. It is installed with an update hook. The view can be disabled and it will fallback to using the default list builder.

Also access checking is now implemented correctly for the user review list and the list of reviews is based on the user id in the route instead of the current user so admins looking at a user's review list will see the correct list.

Cardinal Cruise Authentication is no longer available. There is no replacement.
If Visa protect is in fact a 3DS solution it would likely need a new integration written for it.

Authorize.net has no desire to support 3DS.
https://support.authorize.net/knowledgebase/Knowledgearticle/?code=00000...

They recommend using another payment gateway if you need 3DS support...

Apparently lcobucci/clock was updated to support PHP 8.3 and 8.4 which made this module compatible with Drupal 11.

I have double checked this on a fresh Drupal 11 project.

Still would be a good idea to upgrade to a newer version eventually.

All validation tests are green now.

• Tested module on Drupal 10 and 11.
• Tested Google Maps provider.
• Tested Mapbox Provider.
• France provider has no code changes so did not test.
• Did not test PostCH because could not figure out how to get a test account.

If someone is able to test PostCH that would be appreciated.

Otherwise this is ready for review and commit.

rhovland → created an issue.

rhovland → created an issue.

A token can be easily harvested from a public page where nobody needs to be logged in and then used to authorize automated requests to the json endpoint. How would we handle rotating that token? What happens when it's rotated between user requests?

This doesn't seem to be an issue anymore, at least with the google maps provider. Can you provide more information about what provider you are using?

I'm guessing this was related to this issue which is no longer applicable. 🐛 Google Maps integration incorrectly prepopulates address fields Closed: outdated

Please reopen if this is still a problem on the latest version of the module.

The google maps provider was rewritten and I'm not seeing any of these problems with it. Closing as outdated. Please reopen if you can reproduce any of these issues in the latest release.

The new google maps provider is much simpler and works with the Places API so this is now no longer relevant.

This is no longer applicable to the google maps plugin as it filters by country now. As there was no input provided if this effects other providers going to close this. Please reopen if this affects you.

The policy in question is called security advisory coverage meaning projects are being opted into the advisory process which includes a private issue section for potential security related issues.

It is not a guarantee of security of a module. It means a module has opted into using a private queue for security issues.

The only requirements to opt a module in is that it is in a finished state (stable).

The only requirements for a user to opt a module they maintain into security advisory coverage is them demonstrating they know how to write secure code. If they don't know how to do this then security issues being sent to a private queue only delays disclosure as there is nobody there to fix it (don't know how to write secure code).

If a project opted into security advisory coverage doesn't fix reported security issues then it is removed from security advisory coverage and the issues are made public.

If a maintainer has failed to do this presumably they would have the ability to opt-in revoked as well if warranted.

If you feel you were unfairly removed from being able to opt-in modules you maintain into security advisory coverage (private security issue queue) then explain why. Pointing fingers at others does not contribute anything but noise for drupal ecosystem maintainers. Unreported security issues in someone's module are not grounds for removal. Failure to fix REPORTED issues in a timely manner are.

rhovland → created an issue.

Cleaned up the language further

Issue is ready for review. Test failure is from the main branch and not related to these changes.

Before:

After:

Oh it looks like when the user is anonymous it already prompts for the name. If the user is logged in it uses "my" instead. That seems silly. At least it could use the username. But I think it's better to have that entry box be there, anon or not, as the username isn't always the person's name or a useful identifier for the recipient.

rhovland → created an issue.

rhovland → created an issue.

rhovland → created an issue.

@goz You got this to successfully write order adjustments? Do you have commerce_api installed?

Ok so after combing through things, we were using #72 🐛 [PP-1] Order's Adjustment can't be normalized and serialized Postponed plus the core Any normalizer patch and it was working.

The issue seems to be the final commit never implemented a denormalizer so that PATCH operations can work. The patch we were using had one. Kinda.

diff --git a/modules/order/src/Plugin/Field/FieldType/AdjustmentItem.php b/modules/order/src/Plugin/Field/FieldType/AdjustmentItem.php
index 81ae6c90..cbc96972 100644
--- a/modules/order/src/Plugin/Field/FieldType/AdjustmentItem.php
+++ b/modules/order/src/Plugin/Field/FieldType/AdjustmentItem.php
@@ -3,6 +3,7 @@
 namespace Drupal\commerce_order\Plugin\Field\FieldType;

 use Drupal\commerce_order\Adjustment;
+use Drupal\commerce_price\Price;
 use Drupal\Core\Field\FieldItemBase;
 use Drupal\Core\Field\FieldStorageDefinitionInterface;
 use Drupal\Core\TypedData\DataDefinition;
@@ -48,6 +49,11 @@ class AdjustmentItem extends FieldItemBase {
       // The property definition causes the adjustment to be in 'value' key.
       $values = reset($values);
     }
+    // Used for denormalization.
+    if (is_array($values)) {
+      $values['amount'] = new Price($values['amount']['number'], $values['amount']['currency_code']);
+      $values = new Adjustment($values);
+    }
     if (!$values instanceof Adjustment) {
       $values = NULL;
     }

So as committed this lets you read adjustments via API but not write them.

If I try to send a patch request to https://example.com/jsonapi/commerce_order/default/ab42ab1c-b21f-4da2-9e...

With JSON

{
    "data":{
        "type": "commerce_order--default",
        "id": "ab42ab1c-b21f-4da2-9e9b-82213e622e59",
        "attributes": {
            "adjustments": [
                {
                    "type": "shipping",
                    "label": "Shipping",
                    "amount": {
                        "number": "126.940000",
                        "currency_code": "USD"
                    },
                    "percentage": null,
                    "source_id": "209676",
                    "included": false,
                    "locked": false
                }
            ]
        }
    }
}

I get the response

{
    "jsonapi": {
        "version": "1.0",
        "meta": {
            "links": {
                "self": {
                    "href": "http://jsonapi.org/format/1.0/"
                }
            }
        }
    },
    "data": {
        "type": "commerce_order--default",
        "id": "ab42ab1c-b21f-4da2-9e9b-82213e622e59",
        "links": {
            "self": {
                "href": "https://example.com/jsonapi/commerce_order/default/ab42ab1c-b21f-4da2-9e9b-82213e622e59"
            }
        },
        "attributes": {
            "drupal_internal__order_id": 289391,
            "order_number": "289391",
            "version": 15,
            "mail": "redacted@example.com",
            "ip_address": "208.56.233.218",
            "adjustments": [],
            "total_price": {
                "number": "226.22",
                "currency_code": "USD",
                "formatted": "$226.22"
            },
            "total_paid": {
                "number": "253.160000",
                "currency_code": "USD",
                "formatted": "$253.16"
            },
            "balance": {
                "number": "-26.94",
                "currency_code": "USD",
                "formatted": "-$26.94"
            },
            "state": "shipped",
            "data": {
                "paid_event_dispatched": true
            },
            "locked": false,
            "created": "2024-12-16T21:19:32+00:00",
            "changed": "2025-05-16T20:19:03+00:00",
            "placed": "2024-12-16T21:28:24+00:00",
            "completed": null,
            "customer_comments": null,
            "cart": false,
            "checkout_step": null,
            "field_customer_comments": null,
            "field_customer_motorcycles": [],
            "field_newsletter": false
        },
        "relationships": {
            "commerce_order_type": {
                "data": null,
                "links": {
                    "self": {
                        "href": "https://example.com/jsonapi/commerce_order/default/ab42ab1c-b21f-4da2-9e9b-82213e622e59/relationships/commerce_order_type"
                    }
                }
            },
            "store_id": {
                "data": null,
                "links": {
                    "self": {
                        "href": "https://example.com/jsonapi/commerce_order/default/ab42ab1c-b21f-4da2-9e9b-82213e622e59/relationships/store_id"
                    }
                }
            },
            "uid": {
                "data": {
                    "type": "user--user",
                    "id": "e369285a-16a1-4817-baa5-b4848443a28a",
                    "meta": {
                        "drupal_internal__target_id": 29634
                    }
                },
                "links": {
                    "related": {
                        "href": "https://example.com/jsonapi/commerce_order/default/ab42ab1c-b21f-4da2-9e9b-82213e622e59/uid"
                    },
                    "self": {
                        "href": "https://example.com/jsonapi/commerce_order/default/ab42ab1c-b21f-4da2-9e9b-82213e622e59/relationships/uid"
                    }
                }
            },
            "billing_profile": {
                "data": {
                    "type": "profile--customer",
                    "id": "2cfa79d0-3d74-4c99-bff2-3a26ee115a21",
                    "meta": {
                        "target_revision_id": 363841,
                        "drupal_internal__target_id": 364046
                    }
                },
                "links": {
                    "related": {
                        "href": "https://example.com/jsonapi/commerce_order/default/ab42ab1c-b21f-4da2-9e9b-82213e622e59/billing_profile"
                    },
                    "self": {
                        "href": "https://example.com/jsonapi/commerce_order/default/ab42ab1c-b21f-4da2-9e9b-82213e622e59/relationships/billing_profile"
                    }
                }
            },
            "order_items": {
                "data": [
                    {
                        "type": "commerce_order_item--parts",
                        "id": "55aaae86-e1dc-4c22-943e-7503deef65b7",
                        "meta": {
                            "drupal_internal__target_id": 1073893
                        }
                    }
                ],
                "links": {
                    "related": {
                        "href": "https://example.com/jsonapi/commerce_order/default/ab42ab1c-b21f-4da2-9e9b-82213e622e59/order_items"
                    },
                    "self": {
                        "href": "https://example.com/jsonapi/commerce_order/default/ab42ab1c-b21f-4da2-9e9b-82213e622e59/relationships/order_items"
                    }
                }
            },
            "checkout_flow": {
                "data": null,
                "links": {
                    "self": {
                        "href": "https://example.com/jsonapi/commerce_order/default/ab42ab1c-b21f-4da2-9e9b-82213e622e59/relationships/checkout_flow"
                    }
                }
            },
            "commerce_giftcards": {
                "data": [],
                "links": {
                    "self": {
                        "href": "https://example.com/jsonapi/commerce_order/default/ab42ab1c-b21f-4da2-9e9b-82213e622e59/relationships/commerce_giftcards"
                    }
                }
            },
            "payment_gateway": {
                "data": null,
                "links": {
                    "self": {
                        "href": "https://example.com/jsonapi/commerce_order/default/ab42ab1c-b21f-4da2-9e9b-82213e622e59/relationships/payment_gateway"
                    }
                }
            },
            "payment_method": {
                "data": null,
                "links": {
                    "self": {
                        "href": "https://example.com/jsonapi/commerce_order/default/ab42ab1c-b21f-4da2-9e9b-82213e622e59/relationships/payment_method"
                    }
                }
            },
            "coupons": {
                "data": [],
                "links": {
                    "self": {
                        "href": "https://example.com/jsonapi/commerce_order/default/ab42ab1c-b21f-4da2-9e9b-82213e622e59/relationships/coupons"
                    }
                }
            },
            "shipments": {
                "data": [
                    {
                        "type": "commerce_shipment--default",
                        "id": "86c32de5-5cd6-4542-abaf-23065e0458a1",
                        "meta": {
                            "drupal_internal__target_id": 209706
                        }
                    }
                ],
                "links": {
                    "related": {
                        "href": "https://example.com/jsonapi/commerce_order/default/ab42ab1c-b21f-4da2-9e9b-82213e622e59/shipments"
                    },
                    "self": {
                        "href": "https://example.com/jsonapi/commerce_order/default/ab42ab1c-b21f-4da2-9e9b-82213e622e59/relationships/shipments"
                    }
                }
            }
        }
    },
    "links": {
        "self": {
            "href": "https://example.com/jsonapi/commerce_order/default/ab42ab1c-b21f-4da2-9e9b-82213e622e59"
        }
    }
}

The adjustments are just empty.

So far we have not been able to use PATCH to replace adjustments. They go poof with no error returned. This is on non-draft orders.

I will update our patch on our test site and make sure the removal of AdjustmentPropertyDefinition didn't mess anything up.

rhovland → made their first commit to this issue’s fork.

Here's a preview of how the customer's My Reviews page looks (in a bootstrap theme):

The action links for a user that has already left a review on a product's review page:

Some notes about how the user facing entity list is handled:
The default entity list builder is designed for administrative use. It does not filter the list based on the current user.
The columns presented in the list are relevant for admin users but mostly useless for customers.
As a result it makes more sense to create a separate list builder for customers since the feature set is completely different.

The user list is a route that points to the ProductReviewController to render a page.
The overviewPage() function that gets called to render a page uses the createHandlerInstance() feature of EntityTypeManagerInterface to set a custom handler, in this case a list builder. It then returns a rendered list built by the Entity system.

The one dangling question comes from the comment on createHandlerInstance()

* Usually \Drupal\Core\Entity\EntityTypeManagerInterface::getHandler() is
* preferred since that method has additional checking that the class exists
* and has static caches.

In this case the class always exists because it's part of the module so I'd think a check for that would be unnecessary.
That leaves the question of is this page being output cached? Or do we need to manually cache it?

While it's not clear if assigning storage to a property from a create method has the same problems as when it's done in a constructor I moved to storing the entity type manager in a property instead and querying storage inside the function that uses it just to be safe.

So on our site we're writing new adjustments through the API, not just updating existing ones so we need all of those extra properties to be able to set the characteristics of the adjustment.

Moved the last two commits to separate issues so they can be further evaluated/tested/expanded upon
📌 Safely extend plugin classes Active
📌 Code quality fixes Active