- 🇬🇧United Kingdom jonathanshaw Stroud, UK
Thinking about your options:
maybe the storage choice doesn't matter too much, because you can wrap any storage solution even a "hacky" one in a pretty API easily enough.Maybe the hard thing is usually the UI. In this case, I'm thinking about how this manifests in a subscription form and how easy that is to customise. In this case though I can't see a lot to choose between the options. We will need a custom widget or equivalent regardless.
I was tempted by option C, but the WTF is going to be pretty nasty.
Option A seems pretty sane. I'm assuming you mean not just a new database table, but a new simple SimplenewsSubscriptionEvent entity type. And then for convenience you'd reference the active subscriptions from the subscriptions entity reference field on the subscriber.
- 🇬🇧United Kingdom adamps
maybe the storage choice doesn't matter too much, because you can wrap any storage solution even a "hacky" one in a pretty API easily enough.
The subscription history is complex, allowing for multiple data sets per newsletter including value, date, uid, source, etc. For this part then yes I mostly agree with your remark, although the data will be accessed by views in addition to through the API.
The current subscription status is on/off per newsletter. Although we have an API (
SubscriberInterface::subscribe()
andSubscriptionManagerInterface::subscribe()
), we want it to act like a field, so it can't be hidden/abstracted. It's accessed through the entity field interface, through entity queries, through direct SQL queries, and through views. Some of these are performance critical for sending newsletters. The fact that the current code overloads the field to store partial history requires various hacks elsewhere in the code to then try and hide this history.My proposal is to make the subscriber field back into a normal EntityReferenceItem. We can then remove the above hacks. I am aware that this duplicates the most recent history item, however I feel it's worth it:
- removes hacks in widgets and form code
- maximises back-compatibility: RecipientHandler implementations mostly just need to delete 1 line of code
- maximises performance: RecipientHandler implementations are a simple query of the subscriptions field
- clean design: the history component is well-contained with an API; the current state acts like a simple field
I'm assuming you mean not just a new database table, but a new simple SimplenewsSubscriptionEvent entity type.
Actually I had been imagining a table, but you are probably right to have an entity type.
- Status changed to Needs review
over 1 year ago 4:27pm 21 September 2023 - last update
over 1 year ago 17 pass, 14 fail - 🇬🇧United Kingdom adamps
Here's a work in progress patch that removes a lot of code. It doesn't yet try to add the new subscription history entity.
The last submitted patch, 31: simplenews.track-history.3035367-31.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.- last update
over 1 year ago 17 pass, 13 fail The last submitted patch, 33: simplenews.track-history.3035367-33.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.- last update
over 1 year ago 20 pass, 10 fail The last submitted patch, 35: simplenews.track-history.3035367-35.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.- last update
over 1 year ago 20 pass, 9 fail The last submitted patch, 38: simplenews.track-history.3035367-38.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.- last update
over 1 year ago 42 pass, 4 fail The last submitted patch, 40: simplenews.track-history.3035367-40.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.- last update
over 1 year ago 42 pass, 3 fail The last submitted patch, 42: simplenews.track-history.3035367-42.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.- last update
over 1 year ago 42 pass, 3 fail The last submitted patch, 44: simplenews.track-history.3035367-44.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.- last update
over 1 year ago 53 pass, 2 fail The last submitted patch, 46: simplenews.track-history.3035367-46.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.- last update
over 1 year ago 59 pass - 🇬🇧United Kingdom adamps
Part 1 done - removed the old history tracking. Some tests therefore fail, and are now commented out with @@@@
Part 2 next is to add new history tracking, and uncomment the tests
- 🇬🇧United Kingdom adamps
Update hook is unfortunately not working due to ✨ Field type modules cannot maintain their field schema (field type schema change C(R)UD is needed) Needs work
- last update
over 1 year ago 1 pass, 28 fail - 🇬🇧United Kingdom adamps
First go at part 2, missing most of the update hooks
The last submitted patch, 51: simplenews.track-history.3035367-51.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.- last update
over 1 year ago 16 pass, 12 fail The last submitted patch, 53: simplenews.track-history.3035367-53.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.- last update
about 1 year ago 41 pass, 5 fail The last submitted patch, 55: simplenews.track-history.3035367-55.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.- last update
about 1 year ago 53 pass, 2 fail The last submitted patch, 57: simplenews.track-history.3035367-57.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.- last update
about 1 year ago 59 pass - last update
about 1 year ago 59 pass - last update
about 1 year ago 59 pass - last update
about 1 year ago 53 pass, 2 fail The last submitted patch, 62: simplenews.track-history.3035367-62.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.- last update
about 1 year ago 59 pass - Status changed to Fixed
about 1 year ago 1:23pm 1 October 2023 Automatically closed - issue fixed for 2 weeks with no activity.