Merge Requests

More

Recent comments

🇺🇸United States NicholasS

Sorry to report but even with this patch and my other I still can't bulk process all the nodes, it worked intermittently locally but almost never on our dev server. I am just going to remove this module its been too problematic for our site. Hopefully you can get it working one day. I think it has something to do with all the nested language loops in the batch.

AI tools thought the following could be the issue, but after hours of attempting to patch I gave up. But here are some breadcrumbs for the next person.

After reviewing the code, I've identified several critical memory and performance issues:

Major Issues:
Router Lookup Explosion: views_url_alias_get_path_entity_type() is called for every language variant, even when the path is identical (e.g., /node/123). This is extremely expensive.

Repeated Entity Loading: The same entity is loaded multiple times across different languages without caching.

Individual Database Operations: Each save operation performs separate DELETE and INSERT queries instead of batching.

Memory Accumulation: Entity cache pollution from loading multiple translations without cleanup.
🇺🇸United States NicholasS

Can the typehint be added back? Removing it makes my other patch fail to apply https://www.drupal.org/project/views_url_alias/issues/3491389 🐛 Some path alias crash the creation of the views_url_alias table Active

🇺🇸United States NicholasS

MR33 fixes the error that prevents the database update from working correctly, while maintains the original module intent of working with content entities but adds the needed error handling for sites with webform enitities.

🇺🇸United States NicholasS

Can't update to dev version because of this error.

🇺🇸United States NicholasS

Looks like the issue i have is better to post to https://www.drupal.org/project/views_url_alias/issues/3491389 🐛 Some path alias crash the creation of the views_url_alias table Active

🇺🇸United States NicholasS

Great I can help if you get a branch going, I was going to maybe take a stab at it as well.

🇺🇸United States NicholasS

I too think this is how the module should have been setup, All the reports be editable and installed from config when first installing the module, and maybe a library of "Pre-made" prompts people can choose from so those default ones can be improved over time.

🇺🇸United States NicholasS

Also fixed my "The AI CKEditor dialog form is causing a container serialization error because the plugin instances with service dependencies are being stored in the form cache, and when Drupal tries to serialize the form for caching" issue when attempting to create a custom AI Automator, had similar ajax error console in devtools, but no images involved like original poster.

🇺🇸United States NicholasS

Also had to disable the "Linkit profile" for Ckeditors after updating core 10.5 due to the error "Cannot convert undefined or null to object" in CKEditor 5 caused by a bug in the Linkit module version 6.1.6. Specifically, in the JavaScript file index.js at line 122, the code attempts to call Object.assign(args[2], values) where args[2] can be undefined or null. This occurs when the CKEditor 5 link command is executed with fewer than 3 arguments, but the code assumes the third argument (index 2) will always be an object.

When title is empty and clicking insert

🇺🇸United States NicholasS

Relating to another module that had a similar issue and also was fixed.

🇺🇸United States NicholasS

This change has greatly reduced the time consumption of this module

🇺🇸United States NicholasS

Just to report back that yes this patch did indeed help quite a bit with module usage after deployment. Thanks!

🇺🇸United States NicholasS

I could try it if you have a guide for "Field Widget Action + AI Automators" but honestly after testing the filename token seems to be all the context I need, its good enough to get us and our content editors can adjust from there. Plus teaches them to name their images better which helps content maintenance.

🇺🇸United States NicholasS

I experimented a lot trying to get AI to use the filename, but only luck I had was by adding a token so that I can specifically include it in our prompt template. And for the images I tested this seemed to have a very beneficial effect, so this change just adds a filename token.

Along with IMPORTANT: This image has the filename "{{ filename }}" - use keywords from this filename as strong hints about the image content and incorporate relevant context into your description. it has greatly improved our images alt text. So I feel like this shouldn't be too much of a request to add this token.

🇺🇸United States NicholasS

I added an image to help test this feature request. All I get back is variations like "Single-story beige house with garage, front lawn, and trees in the background."

Also tried some prompt engineering but I just don't think it has access to the filename?
11. If the filename of the image is available you can use it for hints for context as to what is shown in that image and work those words into the description.

🇺🇸United States NicholasS

I wonder if the file name could also help situations like #3493133

🇺🇸United States NicholasS

MR121 looks good to me, no errors our Cypress e2e testing suite dropped from 34:41 to 34:02 so I think this hopefully have a performance impact, but I will only truly know really until its on a prod environment with much more sustained traffic to really know.

Thank you @trackleft2

🇺🇸United States NicholasS

Awe ok I see you working on a version of this for 4.0.24, that explains why I could use composer to test the patch on our site, so I will pause my MR121 in favor of your branch. I did switch to the dev branch and was able to test MR121 and it seemed fine to me.

Glad your looking into it thank you.

🇺🇸United States NicholasS

I think these changes should have perf improvement since it bails early for anon users and don't waste time with config calls. But I won't really know until it released to prod, but I don't see it breaking anything locally.

🇺🇸United States NicholasS

@rachel_norfolk Is this a duplicate of 3433027? Because that one says fixed, but when I tested the 3.x-dev I still got an error during the database updates and the primary keys on the tables could not be fixed due to that error. That's why I created MR31

🇺🇸United States NicholasS

merge request !31 - Allowed me to run the Dev branch database update successfully.

----------------- ----------- --------------- ------------------------------------------------------------------------------------------
Module Update ID Type Description
----------------- ----------- --------------- ------------------------------------------------------------------------------------------
views_url_alias 10301 hook_update_n 10301 - Implements hook_update_N(). Install indexes, add new column, add primary key.
views_url_alias 10302 hook_update_n 10302 - Implements hook_update_N(). Reinstall the schema, rebuild views_url_alias table.
----------------- ----------- --------------- ------------------------------------------------------------------------------------------

// Do you wish to run the specified pending updates?: yes.

> [notice] Update started: views_url_alias_update_10301
> [notice] Update completed: views_url_alias_update_10301
> [notice] Update started: views_url_alias_update_10302
> [notice] Update completed: views_url_alias_update_10302
[success] Finished performing updates.

🇺🇸United States NicholasS

So I tried out the dev version but get an error with the Databaseupdate @ruslan_piskarov

Module Update ID Type Description
----------------- ----------- --------------- ------------------------------------------------------------------------------------------
views_url_alias 10301 hook_update_n 10301 - Implements hook_update_N(). Install indexes, add new column, add primary key.
views_url_alias 10302 hook_update_n 10302 - Implements hook_update_N(). Reinstall the schema, rebuild views_url_alias table.
----------------- ----------- --------------- ------------------------------------------------------------------------------------------

// Do you wish to run the specified pending updates?: yes.

> [notice] Update started: views_url_alias_update_10301
> [error] views_url_alias_get_path_entity_type(): Return value must be of type ?Drupal\Core\Entity\ContentEntityInterface, Drupal\webform\Entity\Webform returned
> [error] Update failed: views_url_alias_update_10301
[error] Update aborted by: views_url_alias_update_10301
[error] Finished performing updates.

Failed to run drush updb -y: exit status 1

🇺🇸United States NicholasS

I also got an error related to the primary keys issue, sovled by clearing cache. When adding a content type I got

Drupal\Core\Database\IntegrityConstraintViolationException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '' for key 'views_url_alias.PRIMARY': INSERT INTO "views_url_alias"

Which I think is fixed by having good primary keys

🇺🇸United States NicholasS

Why can't we just do something simpler like this?

  public function getAliasByPath($path, $langcode = NULL) {
    // Normalize the path: handle empty/null paths and ensure proper formatting.
    if (empty($path) || $path === '') {
      $path = '/';
    }

    // Ensure the path starts with a slash. If it doesn't, prepend one.
    if (!str_starts_with($path, '/')) {
      $path = '/' . $path;
    }

That seems to fix things that I tested from the comments above

  • https://www.example.gov//
  • /index.php5
  • /index.php.
  • /index.php.?hTTp://r87.com/n
  • /index.phpI-dont-exist
  • /index.php.php/some/path
🇺🇸United States NicholasS

This also occurs if someone just adds an extra / to the homepage, for example https://www.example.gov// which the htaccess route (3043779-htaccess-no-php-danglers) doesn't cover that situation.

And the other patch (3043779-source-path-has-11.x) results in a new error

The website encountered an unexpected error. Try again later.

Error: Call to a member function get() on null in Drupal\path_alias\AliasManager->getAliasByPath() (line 199 of core/modules/path_alias/src/AliasManager.php).
Drupal\protected_pages\EventSubscriber\ProtectedPagesSubscriber->checkProtectedPage(Object, 'kernel.response', Object)
🇺🇸United States NicholasS

Hi Kevin, yeah its been on my list to see how this module works with the media library and paragraphs. So no there is not a hidden setting, I think I need to probably make some adjustments to have it work with the media library, our site doesn't use media library much but I know others do.

🇺🇸United States NicholasS

Hi Kevin, one workaround you could do right now is use the views_bulk_operations I imagine to just simply bulk update those nodes you want scanned? I haven't tried that but that should queue them up. But keep in mind the free tier in adobe only allows 500 credits per month.

I have been thinking there should be a UI to manually initiate a scan of file(s) but haven't gotten there yet.

🇺🇸United States NicholasS

Summary: GitLab CI Functional Test Failures Due to PHP 8.3+ Deprecations
The Problem

GitLab CI pipeline was failing with 5 functional tests throwing fatal errors due to PHP 8.3+ deprecation warnings:

Root Cause Analysis
PHP 8.3+ Breaking Change: PHP 8.3 deprecated calling get_class() without arguments
Drupal Core Issue: The deprecation was triggered by Drupal core code (StatusMessages->getInfo()) - not your module code
CI vs Local Environment:
Local tests passed fine (15/15 tests including 5 functional)
CI environment treated deprecations as fatal errors
GitLab CI Template Behavior: The Drupal CI templates' deprecation handling was interfering with our attempts to ignore these warnings
Solutions Attempted
Custom .deprecation-ignore.txt - Created patterns to ignore the specific get_class() deprecation
"Weak" mode - Tried SYMFONY_DEPRECATIONS_HELPER: "weak" to treat deprecations as warnings
Core's ignore file - Attempted to use Drupal core's existing deprecation ignore patterns
Multiple configuration approaches - Global variables, job-specific variables, before_script exports
Why Nothing Worked
The Drupal CI templates aggressively override project-specific deprecation settings
Even when SYMFONY_DEPRECATIONS_HELPER was set correctly, it wasn't being passed through to the actual PHPUnit execution
The CI environment was treating unavoidable core deprecations as test failures
Final Solution: Disable Functional Tests

Will revisit functional test when I test Drupal 11

🇺🇸United States NicholasS

nicholass made their first commit to this issue’s fork.

🇺🇸United States NicholasS

Nice now also has dev deps for easy testing like html email support.

🇺🇸United States NicholasS

@swirt - Hey again! Just an FYI we ended up going with a custom module so never went any further with this module. We used the entity ref module to find orphaned content, and sends emails during work hours/days and then archives its after no action by editors. Its helped cut down on un-maintained content. Maybe I can open source it one day, or this module take some of those ideas. Our custom one works more like SharePoint "Renew Content" feature requiring editors to take action to keep content published.

I did start on our custom pdf accessibility one just recently https://www.drupal.org/project/pdf_services

🇺🇸United States NicholasS

Followed https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa... but not seeing a preview link, maybe it takes a while to show up.

🇺🇸United States NicholasS

nicholass created an issue.

🇺🇸United States NicholasS

nicholass created an issue.

🇺🇸United States NicholasS

Patch failing, do we just need to merge the latest 11.x into this branch?

🇺🇸United States NicholasS


For me it logged as having invalid or empty data. So I do wonder if the cache-data is working correctly... I may dig into this a bit deeper today.

🇺🇸United States NicholasS

Thank you! - Given that I introduced this error it seems only proper I test it. Yeah I didn't catch it since it involved having to wait long enough to have a cached response, but I was getting the same error today when developing locally, a work around was to clear cache before running cron.

But I tested the patch and it works.

🇺🇸United States NicholasS


After this change if no start date in query param it does not default back to Dec 1969

🇺🇸United States NicholasS

So I think the point of this module is to have button shown, so I vote to keep it simple just have them all shown in the bar, thats what this patch does.

🇺🇸United States NicholasS

Can someone explain more about the a11y_title, I am not seeing it when editing H5P content. But it does run the update just fine, just don't want to say this is reviewed without understanding the a11y_title.

I was able to update and save a hotspot that previously existed. So maybe that is enough testing.

 -------- ----------- --------------- --------------------------------------------------------------------------- 
  Module   Update ID   Type            Description                                                                
 -------- ----------- --------------- --------------------------------------------------------------------------- 
  h5p      8005        hook_update_n   8005 - Update hook to modify the database schema to include 'a11y_title'.  
 -------- ----------- --------------- --------------------------------------------------------------------------- 


 // Do you wish to run the specified pending updates?: yes.                                                             

>  [notice] Update started: h5p_update_8005
>  [notice] Update completed: h5p_update_8005
 [success] Finished performing updates.
🇺🇸United States NicholasS

I had to combine with a Drush update composer require drupal/h5p:dev-2.0.x-patched drush/drush:^13 -W

But I can confirm this updates h5p so its compatible with PHP 8.3

🇺🇸United States NicholasS

@shaundychko

So were on the latest alpha, I then require dev so I can test your patch, but I also need the patch from #3497789 but no matter what order I can't apply both patches. What am I doing wrong, since https://www.drupal.org/project/h5p/issues/3497789#comment-15941539 💬 Support h5p/h5p-core 1.27 and h5p/h5p-editor 1.25 in H5PDrupal.php Active makes it sound like it should work.

Here is my patch in composer, I can't get both to work

"drupal/h5p": {
                "https://www.drupal.org/project/h5p/issues/3194707": "https://www.drupal.org/files/issues/2021-01-26/h5p-fix-keeping-h5p-content-id-3194707-0.patch",
                "https://www.drupal.org/project/h5p/issues/3309446": "https://git.drupalcode.org/project/h5p/-/merge_requests/27.diff",
                "https://www.drupal.org/project/h5p/issues/3497789": "https://git.drupalcode.org/project/h5p/-/merge_requests/26.diff"
            },
🇺🇸United States NicholasS

@risweb Replying to Comment #17

So after testing with Drupal CMS 1.0 I switched the mail module to "symphony mailer lite" since that seems to be what they went with and is Drupal 11 compatible, Mimemail is not so my sites I guess will have to switch as well.

Replies to:

1. Ok so if the subscriber block is set to a modal, Yeah I can try to get it to have the success message kept in the modal, your right, right now once you complete the modal signup form it just dismisses and the page has the message in it. Not a problem I don't think.

2. I think this was a double HTML escape issue, I also switched to sympohny mailer lite since thats what Drupal CMS choose and I figured the core guys did their research on the best mailer, so see if the most recent version has proper HTML emails.

3. Based on your screenshot page%20notifications%20-%20verification%20email.png is perhaps your "text format" truncating the url, I see the purple text says /verify.... so I think that could be a text format setting issue. Is that the Maximum link text length? Could you test that out an see, none of my testing had truncated links like that. Maybe also this is fixed after I fixed the double escape HTML issue that I mentioned in #2

Thanks for the feedback, I think its getting close!

🇺🇸United States NicholasS

Anytime, glad to have such a responsive maintainer!

🇺🇸United States NicholasS

@risweb #2 should be fixed now.

Anything else off you notice let me know.

🇺🇸United States NicholasS

@risweb

#1 I think was a bug and I fixed it in commit

#2 Yeah I noticed that as well I will have to look into that maybe today or early next week, I can have that message customized for sure, I don't know why it just doesn't show the form again so yeah I will look more into that.

I also plan to test Drupal 11 as well to make sure it works there. So I will have a few more commits I think to add but thanks for the initial review.

Do you see any other features missing that the original module had?

🇺🇸United States NicholasS

Added more features to it such as verified HTML emails are created, allowing hooks and headers and footers so people can customize the email templates as well. Along with an option for the block to be a button and a modal to save on UI real-estate. Also added logic for people who are already subscribed and attempting to re-subscribe.

Tested with a core Drupal 10 install as well, and improved the

🇺🇸United States NicholasS

After testing on our Acquia hosting, this does appear to fix our issue, and I now see GRPC logging after a cron run with debug.

drush cron -vvv --debug

 [info] Starting execution of google_analytics_counter_cron(), execution of webform_scheduled_email_cron() took 5.41ms. [47.08 sec, 110.3 MB]
WARNING: All log messages before absl::InitializeLog() is called are written to STDERR
I0000 00:00:1737655590.172511     303 call_credentials.c:168] GRPC_PHP: call credentials plugin function - begin
I0000 00:00:1737655590.176159     303 call_credentials.c:171] GRPC_PHP: call credentials plugin function - end
 [info] Execution of google_analytics_counter_cron() took 650.16ms. [47.73 sec, 111.84 MB]
I0000 00:00:1737655651.551433     245 call_credentials.c:168] GRPC_PHP: call credentials plugin function - begin
I0000 00:00:1737655651.553439     245 call_credentials.c:171] GRPC_PHP: call credentials plugin function - end
 [info] Retrieved 7350 items from Google Analytics data for paths 1 - 7350. [109.36 sec, 150.36 MB]
 [info] Merged 7349 paths from Google Analytics into the database. [138.03 sec, 150.71 MB]
 [info] Cron run completed. [138.05 sec, 150.4 MB]
🇺🇸United States NicholasS

Full Disclosure I would not have gotten this fixed without Calude.ai but see the new MR22 which has in it a fix that fixes the segfault issue. I will paste an AI explanation from Claude below, as this is a bit over my head.

Steps to reproduce.

1. Add protobuf https://github.com/protocolbuffers/protobuf to your environment, I used ddev as noted above

webimage_extra_packages: [
  "php${DDEV_PHP_VERSION}-dev",
  "php${DDEV_PHP_VERSION}-grpc",
  "php${DDEV_PHP_VERSION}-protobuf"
]

2. Run cron see seg fault.

Claude.ai Explanation as follows:

The root cause involved three interacting issues:

Protobuf Objects and Serialization

When we tried to serialize Google Analytics' response data, we were dealing with special Protobuf objects. These objects aren't regular PHP objects - they're optimized for network communication using Google's Protocol Buffer format. Trying to serialize them directly caused conflicts between PHP's native serialization and Protobuf's internal serialization mechanisms, leading to segmentation faults.

Memory Management

The original approach tried to handle these Protobuf objects as regular PHP arrays, which created memory management problems. The segmentation faults occurred because we were trying to access memory in ways that the Protobuf extension wasn't expecting, especially when trying to serialize these objects for caching.

RepeatedField Type Safety

The final error revealed that we were treating a Google\Protobuf\Internal\RepeatedField (a special Protobuf container type) as a PHP array. This type mismatch caused PHP's array functions to fail because they expect native PHP arrays.

🇺🇸United States NicholasS

Thanks I tried the suggested "zend.max_allowed_stack_size: -1" but did not help :(

Were still trying to fix this, but got a new breakthrough, after many attempts to force the Google/gax to use REST instead of GRPC we still were getting a seg fault (seemed to be around accessing the credentials file), BUT we found once we removed the PHP package protobuf https://github.com/protocolbuffers/protobuf our seg faults went away, so that appears to be causing the issue.

# ddev config.yaml
webimage_extra_packages: [
  "pngcrush",
  "libjpeg-progs",
  "ffmpeg",
  "clamav",
  "php${DDEV_PHP_VERSION}-dev",
  "php${DDEV_PHP_VERSION}-grpc",
  "php${DDEV_PHP_VERSION}-oauth",
  "php${DDEV_PHP_VERSION}-http",
  "php${DDEV_PHP_VERSION}-gmp",
  "php${DDEV_PHP_VERSION}-dba",
  "php${DDEV_PHP_VERSION}-gnupg",
  "php${DDEV_PHP_VERSION}-protobuf",  <--- This is the problem, once removed and "ddev restart" and seg fault is fixed.
  "php${DDEV_PHP_VERSION}-pspell",
  "php${DDEV_PHP_VERSION}-raphf",
  "php${DDEV_PHP_VERSION}-rdkafka",
  "php${DDEV_PHP_VERSION}-ssh2",
  "php${DDEV_PHP_VERSION}-yaml"
]
🇺🇸United States NicholasS

Ok So after a lot more debugging on and off with acquia support, its something related to the Google Vendor gax, I found this comment that talks about a very hard to debug segmentation fault! And I can agree it was very hard :)

https://github.com/googleapis/gax-php/blob/140599cf5eae2432363ce6198e9fd...

And I was able to add enough debugging to pull this out of the codebase

[debug] Getting feed [37.63 sec, 116.37 MB]
WARNING: All log messages before absl::InitializeLog() is called are written to STDERR
I0000 00:00:1736874324.703588 708 call_credentials.c:168] GRPC_PHP: call credentials plugin function - begin
I0000 00:00:1736874324.703698 708 call_credentials.c:171] GRPC_PHP: call credentials plugin function - end
[error] Error: Maximum call stack size of 10436608 bytes (zend.max_allowed_stack_size - zend.reserved_stack_size) reached. Infinite recursion? in Google\ApiCore\CredentialsWrapper->Google\ApiCore\{closure}() (line 244 of /var/www/html/vendor/google/gax/src/CredentialsWrapper.php) #0 [internal function]: Google\ApiCore\CredentialsWrapper->Google\ApiCore\{closure}()
#1 /var/www/html/vendor/grpc/grpc/src/lib/UnaryCall.php(43): Grpc\Call->startBatch()
#2 /var/www/html/vendor/grpc/grpc/src/lib/BaseStub.php(295): Grpc\UnaryCall->start()

And then I found the hosting I am using has these PHP differences

grpc
grpc support => enabled
grpc module version => 1.68.0
grpc.enable_fork_support => 0 => 0
grpc.grpc_trace => no value => no value
grpc.grpc_verbosity => no value => no value
grpc.poll_strategy => no value => no value
fiber.stack_size => no value => no value
fiber.stack_size => no value => no value
zend.max_allowed_stack_size => 0 => 0
zend.reserved_stack_size => 0 => 0

Still don't have a solution, but at least the problem is narrowed down a lot more.

🇺🇸United States NicholasS

Manual Notification: Some people may not want to edit/modify the content which would affect the date, maybe its a reminder that something is the last day or something, so put in a manual notification option.

🇺🇸United States NicholasS

Manual Addition: Most my clients have emails often from other sources like paper signup forms etc

Migration page with auto complete to transfer subscriptions from one entity to another

🇺🇸United States NicholasS

Drupal Status message after verification link is clicked

🇺🇸United States NicholasS

So I migrated all the config I think I can, some of the "Flow" is a bit different so I am not sure the old templates make 100% sense in the new version. Also there is not "manage subscriptions" page, this version simplifies things and all emails just have an unsubscribe link (like slickdeals/camelcamel etc do it). And some of the old v3 emails are just notifications in the newer version.

Here are some screenshots

🇺🇸United States NicholasS

If on Version: 2.0.0-alpha4 add the following patches to your extras in composer.json

"h5p/h5p-editor": {
      "PHP8 notice https://www.drupal.org/project/h5p/issues/3260094": "https://www.drupal.org/files/issues/2022-01-24/Update_H5P-editor_PHP8_compatible.patch",
    "php83 https://www.drupal.org/project/h5p/issues/346465": "https://www.drupal.org/files/issues/2025-01-13/h5p-editor83.patch"
},
"h5p/h5p-core": {
     "php83 https://www.drupal.org/project/h5p/issues/346465": "https://www.drupal.org/files/issues/2025-01-13/h5p-core83.patch"
},

Until the maintainers can get the upstream h5p library updated, watch https://www.drupal.org/project/h5p/issues/3420268 🐛 Support h5p/h5p-core:1.26 and h5p/h5p-editor:1.25 Needs work

🇺🇸United States NicholasS

This issue could also be fixed by just updating the h5p library right to something newer than 1.24.4 ?

🇺🇸United States NicholasS

Recently added Flood protection to the module. The one aspect I think that could use some work is the signup block, would love to make it a button, that spawns a modal(native dialog) and the modal contains the signup form, and posts/updates with error handling in the modal.

But I think a lot of the other areas of the module are solid, so I am going to open a Merge Request for it so it can get some maintainer attention, would love to merge it to a 4.x branch but Gitlab doesn't allow me to do that, so for now I will MR to the 3.x.

🇺🇸United States NicholasS

Would a maintainer please take a look at my initial refactor of this module to v4. And update hook should migrate all the old subscriptions. I choose not to try to migrate all the old settings so user will have to redo their templates etc. But would love some feed on what I have so far. I tested it and so far looks to work pretty well for me.

🇺🇸United States NicholasS

Was just wondering myself why they still haven't updated now that 7 is End of Life. 

🇺🇸United States NicholasS

Got an initial start on the refactor today. Currently it can install, has settings that can be saved, a block to allow subscriptions, a route to see subscriptions, but still needs a lot more work and features. hopefully I get some more time next week.

🇺🇸United States NicholasS

Ok I am going to start a new issue for the refactor of the module, I'll leave this one as is to fix the current issues I experienced with the module.

🇺🇸United States NicholasS

I think I would be willing to refactor this in a 4.x branch and would propose the following: (Just from initial skim of code)

  • Increase Dependency Injection, removing static calls such as `\Drupal::`
  • Convert to a custom entity type, and provide migration path from nodes

    Use Drupal Config management for settings instead of the database to store module/templates, was there a reason for using the database I'm not aware of?

    Instead of fields being manually added, I think it should be more automatic, and really I think you just need a checkbox for the editor, and can use revision log message field for the short editor note? Id have to dive into more why you need a timestamp field.

    Various token/email validation improvements like token expiration, cron cleanup, also don't love the email in the address bar which would then show in server logs, I feel it could be an obscure ID that relates back to an email.

Do you have any strong opinions on any of those?

Production build 0.71.5 2024