Allow a promotion to belong to all stores

+++ b/modules/promotion/commerce_promotion.install
@@ -115,3 +115,29 @@ function commerce_promotion_update_8203() {
+
+/**
+ * Update the 'stores' field.
+ */
+function commerce_promotion_update_8204() {
+  $definition_update_manager = \Drupal::entityDefinitionUpdateManager();
+

We don't need to invoke the field definition manager.

We could just do something like this

// An empty update will flush caches, forcing block_rebuild() to run.

We really just need the entity field definition cache to be rebuilt

Two comments:
The following // An empty update flushes caches, forcing block_rebuild() to run. is weird? I don't really understand what's the relationship between the empty update hook and block_rebuild()?

Also, I'm wondering if we shouldn't hide the "stores", behind a checkbox. It's probably unclear as is whether the promotion is available to all stores, no?

Shouldn't the update hook contain the following instead?

\Drupal::service('entity_field.manager')->clearCachedFieldDefinitions();

There are no storage changes. Just definition updates to the label and required, which requires a field cache rebuild. So maybe we should just call that in post_update with EntityFieldManager

Also, I'm wondering if we shouldn't hide the "stores", behind a checkbox. It's probably unclear as is whether the promotion is available to all stores, no?

So "Limit to specific stores", check that. Then show the entity reference. Otherwise it defaults to all stores. Kind of like specifying and end datetime?

So "Limit to specific stores", check that. Then show the entity reference. Otherwise it defaults to all stores. Kind of like specifying and end datetime?

Exactly, though a bit unsure about the label itself, but I like the idea of being explicit in the UI.

Also, posting what was discussed on Slack regarding the empty update, we probably simply need the following:

\Drupal::service('entity_field.manager')->clearCachedFieldDefinitions();

This is clearer and more explicit (haven't tested though).

The attached patch addresses the previous comments.

The stores checkboxes are now hidden behind a "Limit to specific stores" checkbox.
The EntitySelectWidget was modified to generically support hiding the checkboxes/autocomplete behind a checkbox.

The update hook simply clears the cached field definitions.

I haven't really reviewed the tests closely, focused on making these 2 changes.

This is how it looks now:

Attaching an interdiff.

I wonder if we shouldn't add a margin to the checkbox, because currently it looks like the checkbox belongs to the order types.

Also, when I went back to edit an existing promotion, the checkbox disappeared:

This is actually by design (copied logic used for the end date), the checkbox #access is set to FALSE whent the field has a value.

I added a margin to the checkbox, I can't really reperoduce the warning, but the attached patch should do the trick.

Instead of the margin, might be better to output the checkbox under the "Stores", but it's still bettr than it was with the margin.

The change in massageFormValues() was problematic, it is fixed.

Also keep the checkbox when a promotion references stores already, it feels weird to have it disapear.

Rather than "Limit to specific stores", I think I'm more in favor of "Restrict to specific stores", thoughts?

+1 to Restrict to specific stores. Limit feels weird. Restrict is more forward.

Updated the label, is it good to go now? RTBC? :p

Went ahead and committed the patch! Thanks everyone!!

🥳 beautiful, three years in the making