Convert subscriptions field to a base field

Yes, I think we should do this.

Miro had a lot of craz... interesting (;)) ideas, about making subscriber a standalone concept, and simplenews would just it. I have no idea how you would want to do that, but possibly bundles? In that case, subscriber would have to be a configurable or at least per-bundle field for the simplenews subscriber bundle....

But as I said, I don't really know how that would work, so let's do this :)

Changes made in the core made the tests fail. Should be fixed now...

I extracted the unrelated test fixes from this issue and committed them. The remaining test fails are caused by the change here, I don't know why yet.

Applied the patch to a fresh branch, made a patch from that, retesting

I had a look at this, the problem is that this breaks views integration. Views doesn't generate the views data for base fields with cardinality > 1 that require a dedicated table.

That won't be easy to fix, as the API for field tables is specific to field config entities, not the more generic field (storage) definition interfaces. And EntityViewsData just looks at the defined tables and their columns.

Will ask @dawehner, but I'm considering to postpone this to a later release, it's not worth spending time on right now I think. Writing an upgrade path should't be that hard, as the storage doesn't actually change, we just need to delete the field configs.

+++ b/simplenews.install
@@ -147,3 +147,13 @@ function simplenews_update_8201() {
+/**
+ * Convert simplenews_subscriber.subscriptions to base field.
+ */
+function simplenews_update_8202() {
+  // Cannot use FieldStorageConfig because it throws an exception due to the
+  // clashing base field.
+  Drupal::configFactory()->getEditable('field.field.simplenews_subscriber.simplenews_subscriber.subscriptions')->delete();
+  Drupal::configFactory()->getEditable('field.storage.simplenews_subscriber.subscriptions')->delete();
+}

sure that there are not reported entity field mismatches after the update?

And what about the views configuration, doesn't that need an update path?

And also views config dependencies, e.g. in form/view displays of subscribers? Sites will have trouble exporting/importing their configuration if you don't clean that up.

sure that there are not reported entity field mismatches after the update?

Aha yes there are. Shouldn't be too hard.

And what about the views configuration, And also views config dependencies,

So it could be any view of subscribers; and any entity form/view display. I have no idea how to start on that! Any ideas?

#11

Writing an upgrade path shouldn't be that hard

Famous last words??

# 11

I had a look at this, the problem is that this breaks views integration. Views doesn't generate the views data for base fields with cardinality > 1 that require a dedicated table.

Appears to be fixed by #2846614: Incorrect field name is used in views integration for multi-value base fields

And what about the views configuration, And also views config dependencies

So it could be any view of subscribers; and any entity form/view display. I have no idea how to start on that! Any ideas?

The upgrade path from #2846614: Incorrect field name is used in views integration for multi-value base fields might give ideas. As well as terrify.

I think that we need to be practical for this module - this is not core and we have not released a stable release. This and other issues will need to change some of the schema, fields, etc, and that's why we made a 2.x.

Here is my proposal:

1. We should fix entity field mismatches
2. We should fix the form/view display modes
3. We should fix sites that have kept the default subscriber view
4. Site owners will need to fix their own custom subscriber views as part of migration to 2.x.

Thoughts?? @Berdir OK with you?

Sounds ok, one thing that might be worth checking is commerce, I know they have a similar issue where they do exactly the same, switch a configurable field to a base field, might have some useful things there.

> Writing an upgrade path shouldn't be that hard

Yeah, that was un unfortuate thing to say, was also a very long time ago ;)

+++ b/config/install/views.view.simplenews_subscribers.yml
@@ -173,10 +171,10 @@ display:
           plugin_id: simplenews_user_name
-        subscriptions:
-          id: subscriptions
+        subscriptions_target_id:
+          id: subscriptions_target_id
           table: simplenews_subscriber__subscriptions
-          field: subscriptions
+          field: subscriptions_target_id
           relationship: none

the thing that I find a bit weird actually is why this even changes? The storage should be identical between a multi-value base field and a configurable field, so it might "just" be exposed differently to views. It might be easier to keep the structure in views the same than having to reconfigure it in an update function.

Thanks @Berdir

I know they have a similar issue where they do exactly the same, switch a configurable field to a base field, might have some useful things there.

Ah good idea, I found #3002939: Convert order/product multivalue configurable fields back into base fields. That issue even refers back to this issue and quotes Berdir saying "it shouldn't be that hard"!!! :-)

However I'm not sure about starting the work to try and auto-migrate views as I think some of the other issues might make more drastic changes that could not be auto-migrated. I propose to defer views migration until later when the overall picture is clearer #3059184: View migration for 8.x-1.x to 8.x-2.x. It will be a blocker for a 2.x beta.

It might be easier to keep the structure in views the same than having to reconfigure it in an update function.

Do you have something in mind? In both the old code and the new code, it was core Views code that chose the structure. The Commerce patch also seems to have made the exact same change that we have here. It feels like it will be difficult/risky to try and fight the way Views works and might create problems for the future.