🇳🇴Norway @zaporylie

Restored back the OrderReceiptPolicy enum and removed the additional interface and trait. Generally speaking, I prefer not to break the contract, but we agreed that the risk here is really low, and currently, it's not clear if this feature can be used as a general API.

zaporylie → created an issue.

> I didn't want to touch this method at first for that reason, what I dislike if we don't change the method is we now need to copy the same code potentially in multiple places and we don't have a single entry point for determining whether the receipt should be sent for an order.

I think this calls for a method on the Order class itself. I would leave it as is for now until the need for reusing logic in other places emerges.

> CommunicationPolicyInterface sounds really really generic. We only control the order receipt atm.

It does, and given that we only use it in the receipt-aware scenarios, I unified it to ReceiptPolicy (ReceiptPolicyTrait/SupportsReceiptPolicy).

> We could potentially skip "Default", and rename OrderReceiptPolicy to OrderReceiptPolicyOverride? Or even stick with OrderReceiptPolicy but just omit the default.

I think it's better to skip Default or use it as default instead of NULL.

> With that said, the "transient" policies (AllowOnce, SkipOnce) are transients and can't really be used for the shipments. But I guess the Forbid policy could be somehow carried on if we made the OrderReceiptPolicy an OrderCommunicationPolicy instead?

After going through the code, I am leaning towards defining CommunicationPolicyInterface/Trait for addressing this. I don't see any benefit in particular to keep it order-oriented.

Re MR - I left some in-line comments. Major issue IMHO is the change to the interface method's signature, as that breaks the contract. Therefore I would prefer if we keep the order-type level method and fallback to that when policy returns null, or default in case we decide to keep it.

I don't want to mess up the MR that is now fully functional so I'll branch out and test some refactoring there.

I created a new access checker, as accessCreate doesn't have direct access to contextual route parameters.

I am leaning towards implementing PaymentAccessControlHandler::createAccess to check whether order balance is not null.

Added test coverage for the PaymentOptionsBuilder.

There are 2 things that should likely be addressed:
1) non-reusable payment methods should never be set as default.
2) deleting default payment method should make the next eligible payment method default

Both these points can be done as follow-up(s) and need some extra considerations.

The merge train is riding.

zaporylie → created an issue.

zaporylie → created an issue.

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

If we were to support persistent receipt notification suppression, I think the action link must be hidden and the checkbox in the transition form, at the very least, disabled. While I don't want to bring another field to the order base table, I think that would actually be the cleanest approach. notify - tinyint with enum-based values could do the job:
SUPPRESS_PERMANENTLY = -1
SUPPRESS_TEMPORARILY = 0
ALLOW = 1 (default)

We could also just go for a Boolean field type, given that SUPPRESS_TEMPORARILY would possibly never have to be stored in the database (changed to ALLOW on Order::preSave), but I don't love the fact that it would likely mean we still have to use int for setter/getter and then process it to bool on preSave (-1 => 0, 0 => 1, 1 => 1). and again on OrderStorage::preLoad (0 => -1, 1 => 1). But given tinyint and bool are of the same storage size using signed tinyint would just be cleaner.

I rebased the MR, reviewed it, and provided necessary updates, including fixes to code and tests. The current test actually revealed an issue with the owner check, which is now corrected.

I think it's really convenient, from the OMS point of view, to be able to create shipments and have the order updated accordingly. Having to manually add adjustments would be very inconvenient. Having price adjustments automatically set, but tax adjustments not, could somehow be worse, as it would give a false belief that everything is happening automatically until you realize taxes are not there.

IMHO, this issue could potentially benefit from a change to the OrderRefresh flow that would allow running a subset of order pre-/processors on placed orders by introducing order refresh scopes. Scopes would be defined as tagged service attributes and if missing, in the current release cycle we can assume they are only meant to run for draft orders. However, we could provide scopes for placed or even completed orders and run a curated subset of processors that we know can be run deterministically, and the one copying shipping adjustment is a prime example of such a processor. Tax processor could potentially also be safe to run if we resolve taxes for a specific date (current vs placed).

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

I decided to bump the severity to Critical as items can also be deleted from the cart if one of the availability checkers returns that the item is not available - see AvailabilityOrderProcessor => AvailabilityManager

auto_entitylabel module uses tokens within TranslatableMarkup's placeholder ('@placeholder'), which is what's causing this issue in the first place.

If we add support for an alternative fallback string dynamic replacement token we could leave the old logic in place but document how to use new syntax, gradually deprecating old syntax.

Currently, I solved it like that in the project's custom codebase, and this solution is working well. Whitespaces are supported in case that's someone's concern.

Please also notice that a similar issue was created in auto_entitylabel module, link in Related issues.

IMHO both modules could benefit from improvements: token_or could stop relying on quotes which would also resolve ✨ Allow to use ' as enclosure for using in href attributes Postponed: needs info while auto_entitylabel should stop using TranslatableMarkup in favour of having a translatable config entity.

One way to support this would be to create a generic, dynamic [fallback_value:*] dynamic token, which will stop relying on the double quotes.

zaporylie → created an issue.

zaporylie → created an issue.

One more note here because something similar came up in one of the projects I've been working on recently. In their case, some orders would be created programmatically, and in such a case, all following messaging is supposed to be suppressed. So in that case, the flag would have to be permanent.

So my proposal here is to change from using Boolean values to have a helpful CONST defined on the MailerHelper and have the possibility to set skip_order_receipt to PERMANENT, and in such a case, do not reset the value. We could also use that value to hide the action link. Furthermore commerce_shipping could take advantage and suppress shipment confirmation if needed (TBD).

MR is open and default implementation added for some of the basic shortcodes, including the [random] one that if [random cache=0 /] is used the shortcode will be rendered on every page load (maxAge = 0)

zaporylie → created an issue.

zaporylie → changed the visibility of the branch 3538535-propagate-synchronize-flag to hidden.

zaporylie → created an issue.

The issue mentioned in the summary is indeed related to Layout Builder Expose All Field Blocks as suggested by #4 and is now enforced in Kickstart 5.

Re #5 - I am not sure if I follow the question but it doesn't seem to be related to this support request.

zaporylie → created an issue.

Let's target both getPlacedTime and getCompletedTime. Not sure about setting the return type on the interface though - I am all for having it, but this will break things for those who override/decorate the entity class

I tested the functionality of the patch and here are my findings:

1. Connecting the gateway to a merchant’s Stripe account works correctly, including disconnecting. This was tested using the Stripe Sandbox.
2. If the gateway is first saved as "Stripe Connect" and later switched to "API Keys," it will silently revert back to "Stripe Connect" if the secret key is left empty. I believe additional validation should be added to prevent this.
3. Additionally, if the merchant's account is already connected via "Connect with Stripe," switching the mode to "API Keys" invalidates the connection, but the "Stripe Connect" option remains selected. This could be confusing and may need better handling in the UI or logic.
4. When the account is connected, the option to switch between "API Keys" and "Stripe Connect" should be disabled to prevent accidental misconfiguration.
5. Placing the Connect/Disconnect button next to the "Mode" setting on the payment gateways page feels visually inconsistent. Consider replacing it with operation links for "Connect" and "Disconnect," and showing a styled button on the confirmation screen instead.
6. The confirmation page shown during the "Connect with Stripe" flow could also serve as the landing page after submitting a new or existing payment gateway with "Stripe Connect" selected—if the merchant's account hasn’t been connected yet.
7. Disconnecting a Stripe gateway causes a WSOD on the checkout page if the user selects a payment method that was previously created with Stripe. This should be handled more gracefully.
8. On a positive note, connecting, disconnecting, and reconnecting with Stripe Connect preserves access to previously created payment methods, which works well.

MR looks good to me. I tested it with both Drupal CMS and Commerce Kickstart, and it looks decent in both cases. The implementation is straightforward. The only thing that raises a yellow flag for me is the new condition for the coupon redemption form, but it feels more like fixing a bug than creating a functional regression, although I am sure this could potentially break someone's workflow.

I wonder if we should prepare a change record?

LGTM. Thanks @vegardjo

zaporylie → created an issue.

Given this will use the Intent API please also consider adding SupportsZeroBalanceOrderInterface simultaneously. Thanks.

Re #13: I would expect for unavailable purchasable entities to be available to the wishlist, at least to some extent. If the item is not available, it means (at least in my head) it is not purchasable. But one might add it to the wish list to track whether it becomes available in the future.

Thanks for keeping pushing for the release and sorry for the delay in tagging one.

I just tagged 8.x-1.3 and published the release https://www.drupal.org/project/telephone_formatter/releases/8.x-1.3 →

I gave some more thought to the proposal in #7. While it makes sense within the scope of the current issue, I believe we should keep the implementation as it is and instead adjust the scope.

In my view, we're aiming to replicate the default behavior of the Add to Cart form—which, like with any other entity type in Drupal, runs validation before creating an order item.

A good example of why this matters: imagine a Commerce site with multiple product types, each tied to its own order item type. Some of those order item types have additional required fields. Full validation ensures we don’t end up creating invalid order items when the Add to Cart form would normally prompt for more data before proceeding.

LGTM

The MR adopts the same messaging approach as the standard Add to Cart form. It delegates responsibility to the checker to override the generic message with a more specific one that clearly explains why the item couldn’t be added to the cart.

For some reason, I didn't know about this module until recently. And.. yeah.. It's a great module!

I found this issue with 🐛 Synonyms autocomplete field does not fail if unknown value entered Fixed . The requirement I am facing is to be able to create custom content entities (not terms) and support synonyms at the same time. Or, in short, respect the setting of the entity reference handler.

We had an internal discussion that triggered a small but mighty change to the scope of this issue. The idea is to toggle availability checkers on a per-order-item-type basis.
The MR is now updated accordingly.

I’ve updated the PR to use the attribute approach. Generally, it’s working, but there’s a major flaw in the architecture.

Because of how the container works, a single class can represent multiple services. With an immutable Attribute, we lose the ability to vary metadata per service instance.

Given that, I’m considering moving back to the interface approach but using a Metadata value object to keep things cleaner and more structured.

I came to the same conclusion about utilizing a custom Metadata attribute. Additionally, I believe the metadata could also include an ID (separate from the service name), which would then be used in the config sequence to mark the checker as enabled or disabled.

I also think that filtering of checkers should happen in the constructor (as originally proposed) to avoid the performance overhead of limiting the list repeatedly during each call to AvailabilityManagerInterface::check.

The main advantage of using tagged services over plugins is better performance, as tagged services are collected during the container build phase. Introducing extra filtering steps at runtime for every pass would be suboptimal. This approach would only make sense if we were using plugins, which we are not.

I published the MR with the proof of concept. Below is a screenshot from UI with the exposed checker from ✨ Add an availability checker which checks if the entity is accessible or not Needs work .

This is a valid addition. However, with 3.x already released, we missed the opportunity to introduce it in a new major version—especially since it might not be backward compatible. That said, this is actually a good thing. We should ideally avoid introducing backward-incompatible changes even between major versions, when possible.

I believe this new checker can still be added, as long as it only affects new sites and existing sites retain their current behavior.

To make it easier for our core audience—ambitious site builders—to enable or disable availability rules and checkers as needed, I’ve proposed ✨ Expose Availability Checkers in the Admin UI Active .

zaporylie → created an issue. See original summary → .

1. We not only use the availability manager in the context of the order processor but also in the context of validation constraints. That's what prevents non-available items from being added to the cart.
2. We can replicate the context in the cart links module and use it to validate the order item, in fact if we call validate on the order item, that would trigger the same code/behavior as the add to cart form.

I added MR with a proposal.

zaporylie → created an issue.

zaporylie → created an issue.

Chiming in as this is a blocker for my contrib module that aims to inject an additional link to be shown next to the installed module. Currently, the list of potential links is fixed to only 3 options - help, permissions, and configuration. IMHO, the module interface should make it possible to inject any link. Dropping support for icons could and moving onward to dropdown, operation-style links could make that much easier.

I am not sure if skipping default content support for modules was intentional, so I created this issue to also serve as documentation in case the issue status is changed to "won't fix".

zaporylie → created an issue.

MR needs to be rebased.

I don't feel comfortable about this behavior in the context of a promotion form, otherwise all should be good.

As part of my MR I only added support to selected entities:
- order
- product
- product_variation
- promotion
- coupon
- store

Some notes:
- Not all content entities support sidebars (ie. coupon), which makes the form look a bit weird (very subjective opinion)
- For config entities we are better off without gin content form support as these forms are rather simple, featuring only handful of fields
- Some content entities are not to be edited in the context of admin UI (ie. payment_method) hence no need to support it either

I removed support for D10; otherwise, LGTM. Tested and works precisely like Centarro Claro. I also checked that the theme is not installed nor available after the site is installed which is expected. Thanks

I appreciate the feedback in #10 and #11. Routing recipe application d.org update requests to the queue, with the update module acting as the queue consumer, sounds like a clean approach that respects global tracking settings.

I’m curious about one more thing — the issue summary mentioned the name as one of the properties featured in the request. I’d like to clarify two points:
- The recipe directory name—which is essentially what the recipe name comes down to—has only a loose connection with the project on d.org. I believe we should only send an application update if the recipe includes a composer.json file, so we can verify that it belongs to the drupal/ namespace as outlined in one of the conditions in the issue summary. This would filter out all custom recipes added at the project level (i.e., not available on d.org even if the recipe name matches a general project on d.org), as well as recipes under other vendor namespaces.
- This also means that all core recipes would never be tracked. Are we okay with that, or should we support core recipes too? If so, we’d need to collect version information from the drupal/core package.

The site name is set by the installer based on the provided input. I just tested it with and without the demo content, and my site was named with the name I provided.

I wonder about a scenario in which a recipe is applied multiple times because it might be a dependency for many other recipes. That would be a common case for starter/base recipes. Said recipes can be deduplicated, which is an approach the recipe installer kit is promoting, or simply applied multiple times. The main issue I see here is inconsistency, which results in slightly off telemetry data.

Re #5: while I agree that the proposed approach is clean, I wonder how this could be respected if the recipe is applied via installer (no option to opt-in/out) or the site is installed from the recipe (drush si ../recipes/drupal_cms_starter).

Re #7: Recipe Tracker (thanks for mentioning it here) is meant to track the application of recipes locally, within your Drupal instance, and never sends any data outside the Drupal instance context.

Thanks for the catch, and if you are interested in contributing, feel free to propose a patch/MR. Once again, thanks!

I discovered that most of the duplications I am seeing are coming from strict comparison when batch tasks are containing instances of Recipe object representing the same Recipe.

All blockers were addressed. Therefore, closing as fixed.

zaporylie → created an issue.

All issues were addressed. I will be tagging the stable release soon.

I tested this MR and happy to report that the horizontal scroll is now gone and no incorrect overlaying could be spotted anywhere. LGTM

zaporylie → created an issue.

That works, the validation is happy, I am happy. Thanks :)

I set it up locally and the MR seems to do the job. Local tasks stick to the right and are visible most of the time. Only when off canvas sidebar is opened do they get hidden but at least they are not in the way which is a major improvement. Thanks!

zaporylie → created an issue.

zaporylie → created an issue.

https://www.drupal.org/project/commerce_recipe_core → is now in place.

Fixed via 🌱 Consider using Recipe Installer Kit Active

Fixed in https://github.com/centarro/commerce_kickstart/pull/7

zaporylie → created an issue.

This was fixed as part of 📌 Convert commerce_kickstart profile installation to recipe Active . We may want to expose the currency selector in the installer down the road but for now we decided not go this way for the initial release.

This will likely be fixed as part of 🌱 Consider using Recipe Installer Kit Active . Postponing for now.

zaporylie → created an issue.

I removed Commerce Product Tax from the recipe in https://git.drupalcode.org/project/commerce_kickstart_demo/-/commit/bdea...