🇮🇪Ireland @lostcarpark

I've reviewed and tested locally, and I'm happy with this change.

One caveat: I would like to move these functions out of the .module file and into services, which would allow them to use dependency injection, and would avoid the function being loaded until it's needed. There would be an argument for skipping this intermediate service and making that change in this issue.

On the other hand, if we decide to leave the services to a follow on, I would be tempted to merge this, and rebase 📌 Add static caching to scheduled_publish_get_node_fields() to improve performance when displaying multiple nodes Needs work .

Merged and verified tests passed. Moving to fixed.

As there have been no objections, and the current versions are still available for D9, I'm moving this to RTBC.

Thanks for the feedback!

It sounds like using Drupal caching here is a bad idea, but static caching should give some small benefit, so sorry if I pushed things in the wrong direction.

If it's worth keeping caching in scheduled_publish_get_node_fields we should revert back to the earlier static variable. However, if the benefit is very marginal, it might be better to drop caching to avoid adding complexity.

For scheduled_publish_get_node_workflow_states, is it worth using static cache here, or should we drop that altogether? I'm feeling the latter.

I think the test cases are still useful.

@berder, I was torn on that, as the module still supports Drupal 9. There is an issue to drop D9 support, and once that's closed I want to migrate all hooks to OOP. I think we're ruling out adding new hooks now, anyway.

Awarding contribution credit...

feat: #3557266 Mentor Meeting - 12 November 2025

By: chrisdarke
By: mradcliffe
By: alvarito75
By: lostcarpark
By: jimmycann
By: antojose
By: kristen pol
By: igorgoncalves
By: richo_au
By: xjm

feat: #3556286 Mentor Meeting - 5 November 2025

By: lostcarpark
By: xjm
By: anmolgoyal74
By: alvarito75
By: antojose
By: kristen pol
By: jimmycann
By: richo_au

Updated contribution record.

feat: #3555092 Mentor Meeting - 29 October 2025

By: alvarito75
By: anmolgoyal74
By: chrisdarke
By: rodrigoaguilera
By: antojose
By: xjm
By: igorgoncalves
By: kristen pol
By: shriaas
By: salimlakhani

feat: #3553653 Issue Triage at DrupalCon Vienna 2025

By: lostcarpark
By: rodrigoaguilera
By: chrisdarke
By: rachel_norfolk
By: igorgoncalves
By: xjm
By: dinarcon
By: vijaycs85
By: megachriz
By: yautja_cetanu
By: kristen pol

feat: #3553660 Mentor Orientation at DrupalCon Vienna 2025 - Tue 14th Oct 2025 14:45-15:30

By: lostcarpark
By: megachriz
By: rachel_norfolk
By: igorgoncalves
By: chrisdarke
By: michaellenahan
By: anmolgoyal74
By: antojose
By: salimlakhani

feat: #3553661 Mentor Orientation at DrupalCon Vienna 2025 - Wed 15th Oct 2025 10:30-11:15

By: lostcarpark
By: rachel_norfolk
By: chrisdarke
By: michaellenahan
By: john cook

feat: #3553665 Mentoring Booth at DrupalCon Vienna 2025 on Tue 14th Oct 2025

By: lostcarpark
By: igorgoncalves
By: rodrigoaguilera
By: rachel_norfolk
By: chrisdarke
By: antojose

feat: #3553666 Mentoring Booth at DrupalCon Vienna 2025 on Wed 15th Oct 2025

By: lostcarpark
By: rodrigoaguilera
By: rachel_norfolk
By: chrisdarke
By: igorgoncalves
By: anmolgoyal74

feat: #3553667 Mentoring Booth at DrupalCon Vienna 2025 on Thu 16th Oct 2025

By: lostcarpark
By: rodrigoaguilera
By: rachel_norfolk
By: chrisdarke
By: igorgoncalves
By: antojose

I have opened !53, which removes the cache tags, and replaces with hooks to explicitly invalidate the cache entries.

For scheduled_publish_get_node_fields, I used hook_ENTITY_TYPE_insert and hook_ENTITY_TYPE_delete on field_config, which trigger when a field is added or deleted from a bundle. In this case, I looped through all the active languages, and invalidated the cache entry for each.

For scheduled_publish_get_node_workflow_states, I used hook_ENTITY_TYPE_update on workflow to identify when a workflow was updated. I invalidate the tag for the language of the updated workflow.

The tests are unchanged from !33, and are passing. I also verified they also pass for "Test changes only".

I think !53 will be more performant, but I think there is a risk I have missed case where they should be invalidated.

From that perspective, !33 may be slightly lower risk. However, the only times the cache need refreshing when structural changes to fields or workflows are made, so in the worst case, a manual cache rebuild should resolve problems.

Added @chrisdarke and @lostcarpark from Google Docs history.

I took part.

Talking with @berder on Slack, they suggest: "also, keep in mind that cache tags aren't free. every unique cache tag per page needs to be looked up on cache get. if you only have a single cache item with a fixed ID, it might be better to implement an entity hook and explicitly invalidate your cache instead of the cache tag lookup".

I'm going to try that in a separate branch and see what we think.

I see your point about \is_array(). If we wanted the most performant comparison, we could probably just use if ($cachedStates == FALSE), since cache()->get() returns FALSE if the cache entry doesn't exist, and there should never be a non-array in the cached data.

I think moving the renaming of the functions into a separate issue is a good call! I hadn't considered it would break test only changes!

I agree this is a problem, but I'm not sure I understand why it's happening, or whether it's caused by Scheduled Publish. Surely it's the Workflows module that's responsible for the transition, and for setting the status?

I will need to test with just Workflows and verify it doesn't happen with Workflows alone.

I've found the correct tag for the workflow states: config:workflow_list.

I've extended the workflow states test to add new states to the "editorial" workflow, and add a second workflow for the "article" content type. This verifies that the cache is invalidating when workflows are amended or added.

I've also run the new tests against the uncached version of the functions. It would have been ideal to use the "Test only changes" functionality in GitlabCS to do this, but the addition of the underscore prefix means this won't work. I did test locally, with all changes besides the underscores removed.

Appreciate if someone could review so we can merge.

I was there!

I discovered a mistake in the cache logic; \Drupal::cache()->get($cid) returns a cache object, not the cached value, which is in the ->data property.

I've added testGetNodeFields, which tests _scheduled_publish_get_node_fields. It uses the default NodeTest setup, which creates a Page node type, and checks one Scheduled Publish field exists. It then adds an Article content type with its own separate scheduled publish field, then checked that the function returns both fields.

I added caching to _scheduled_publish_get_node_workflow_states, using the plugin_manager_cache_clear tag, which seems to be cleared in the Workflows module, but I'm not sure that's the correct tag, so I would appreciate if someone could take a look at that.

I've also added testGetWorkflowStates to test the workflow states. I think ideally it would add an additional workflow state, and verify it is returned by a second call (this would also verify that the cache tag is cleared when the workflow changes).

Setting to Needs review, but happy to go pack to NW if anyone isn''t happy with it.

Discussed with @ultimike in DrupalEasy Office Hours today, and think I have a plan for testing this change.

1. scheduled_publish_get_node_fields is a utility function, not a hook, so it should be prefixed with an underscore.
2. scheduled_publish_get_node_workflow_states has very similar functionality, so would probably make sense to also cache it's output. It should also have an underscore prefix.
3. createNodeType function exists in both FormatterTest and NodeTest, so should be in a trait (so it can be used by new test).
4. Add new test cases to NodeTest to test scheduled_publish_get_node_fields and scheduled_publish_get_node_workflow_states by ensuring they return the expected fields and workflow states for several node setups.
5. Verify that new tests pass both before and after the new caching.

We also discussed the utility functions, and agreed that longer term they should be moved into a service to better handle dependency injection and separate them from the .module file, but that should be a follow-on issue.

That seems valid. I don't think we need to test Drupal core's caching system!

Rebased off 11.x. Tests seem to be passing again.

Just to clarify, I wasn't suggesting <!--break--> shouldn't be supported, just maybe it should be an option rather than something that's always enabled.

Thank you for working on this. The cache change looks good to me.

I wonder have you any thoughts on how we could add test coverage for this?

I cannot see a good reason for QueueFactory not to implement QueueFactoryInterface. It implements the method specified by QueueFactoryInterface.

Just to check, I have added implements QueueFactoryInterface, and ran the tests in the CI. A FunctionalJavascript test failed the first time I ran it, but passed when I reran.

It seems to me that it should implement the interface, but someone wiser than me should review to confirm.

Rebased after merge of ✨ Dispatch event when an entity is changing state on schedule Active . Also removed 9.x from test module.

Thank you for all your great work on this issue. I have opened !51 against the 4.2.x branch, and the change cleanly cherry-picked into it.

Moving to fixed.

Thanks again!

Didn't notice the MR was created against 4.0 before merging. Have opened one against 4.2.x bramch.

I've reviewed the code and done some manual testing. Everything looks good to me. Nice to see it in ECA!

Moving to RTBC.

I attended!

We have an issue open to move to Object Oriented hooks ( 📌 Convert procedural hooks to object oriented hooks Active ), which I believe requires Drupal 10.1, and a minimum PHP version of 8.1, so I think this seems a good target for now. I'd also like to start moving to autowiring of services, which also requires 10.1.

I'm a big fan of ECA, so would be very much like to see support for it added. I'm happy for it to be either part of this issue, or a follow-up, whichever suits you best.

Thanks for working on this, @Souvik_Banerjee.

Change merged successfully, contribution credit awarded and moving to fixed.

Reviewed and tested locally, and it looks great.

Just a reminder to set the issue to "needs review" when your changes are ready.

Moving to RTBC now.

Drupal 11.3 is expected in December, when 11.1 will become unsupported, so while keeping our core_version_requirement on 11.1 is fine, people should upgrade to at least 11.2 if possible.

We could consider increasing our minimum to 11.3 either in mid-2026 when 11.4 is released and 11.2 unsupported, or in December 2026 when 12 is expected to release, and the supported versions of 11 will be 11.4 and 11.5 (and all versions of D10 will be unsupported).

lostcarpark → created an issue.

Very good work! I have made a few small suggestions. We should then be ready to merge.

lostcarpark → created an issue.

I thought it would be helpful to review the plugins, and check the Drupal version annotations are supported from:

• Actions - 10.2
• Blocks - 10.2
• Derivative - no attribute required
• FieldFormatter - 10.3
• FieldType - 10.3
• FieldWidget - 10.3
• MigrateField - 10.3
• MigrateProcess - 10.3
• MigrateSource - 11.2
• QueueWorker - 10.3
• Constraint - 10.3
• RegistrationConstraintBase - defined within module
• ViewsArea - 10.3
• ViewsArgumentDefault - 10.3
• ViewsField - 10.3
• ViewsFilter - 10.3
• ViewsRelationship - 10.3
• WorkflowType - 10.3

Currently the module supports 10.3, so plugins that were converted after that can not me migrated to annotations yet. As far as I can see the only plugin type affected for this issue is MigrateSource, which requires 11.2. I would suggest migrating the others to annotations, and leaving MigrateSource for a follow-on issue when support for Drupal 10 is dropped.

Entities are not plugins, but also require 11.1 or later for annotations, so can't be migrated yet.

I attended!

Thanks for working on this @ritarshi_chakraborty!

Successfully merged, contribution credit awarded, and moved to fixed.

I love how simple this change turned out to be, while improving the flexibility of the module quite a lot. Thank you for working on it.

I have reviewed the change and tested locally, and I'm happy that it's working correctly.

Moving to RTBC.

lostcarpark → created an issue.

Hi @yoa,

We have now removed the "final" class declarations, so subclassing should be possible.

We have also added two permissions (per verify form), "Access verification form" and "Skip verification form".

You can grant "Skip verification" permission to roles that will be directed to bypass the form, so it will be shown to logged in users who don't have that role.

The "Access verification" permission doesn't do anything yet, but that will be the next issue. That will let you determine who can access the form. I think you only want logged in users to be able to verify, so if you only grant this permission to "Authenticated users", anonymous users won't see it.

These are currently only in the Dev release, but when we have a few more issues completed, we'll look at moving to a Beta release.

Thank you for working on this, @ritarshi_chakraborty.

Change merged and contribution credit awarded.

Thank you for working on this.

I have reviewed the change, and I have carried out a manual test, and I'm happy this is working as expected.

📌 Create custom permissions per verification form Active is now merged and fixed, so this issue is safe to proceed with.

Merged change and verified merge train passed successfully.

Moving to fixed and awarding contribution credit.

Thank you @nidhi27 and @ritarshi_chakraborty for working on this. I think it's a nice improvement to the module.

I have reviewed the changes, and run the tests locally. I also ran the "test only changes" and verified it failed. I did some manual testing to verify the permissions are created for each verification form.

Ah, so close!
Just need to update the docblock comment with the new parameter name.

You are almost there, but you need to change the name of the constructor parameter from $entity_type_manager to the property name $entityTypeManager.

Thanks for your work on this @ritarshi_chakraborty.

Accidentally selected RTBC before I noticed the property promotion issue.

Sorry, I missed one small thing on my previous review. The $entityTypeManager currently uses a separate declaration. In PHP 8 this can be replace by property promotion from the constructor.

lostcarpark → created an issue.

This is almost perfect. The only minor issue is that when using AutowireTrait, you don't need a create() function. Autowiring generates one automatically. Just delete create() and we should be ready to merge.

Thanks!

You're right, I jumped the gun on the service tag.

I have updated the Remaining tasks to specify permission_callbacks instead.

Thank you. 3.x now marked unsupported.

I was there!

lostcarpark → created an issue.

Merged successfully.

Thanks for your work on this issue, @tanushree gupta.

Moving to fixed.

I have reviewed the changes, and carried out some manual testing. I also ran the automated tests locally. Everything looks good to me.

Moving to RTBC.

lostcarpark → created an issue.

I will be at DrupalCamp Scotland on Friday, and I will happily buy any Canvas developers present a drink!

Oops, I think the issue I cloned was fixed, so this was marked fixed.

Latest version of change looks perfect to me. Not sure if test case is needed for this.

lostcarpark → created an issue.

Thank you for working on this @souvik_banerjee.

Merged and moved to fixed.

Verified form is removed. As form never had a route, I thought it extremely unlikely that test results would be affect. Nevertheless, I have verified test results are unaffected.

Change merged.

Moving to Fixed.

Thanks for working on this, @tanushree gupta.

I've verified example removed from schema and tests all pass. I've also done a quick manual test.

Moving to RTBC.

Updated MR to target 1.1.x.

Thanks for your work on this, @nidhi27.

Merged successfully and moving to fixed.

Please review the contribution record and add an organisation or mark your time as volunteering if appropriate. You can set defaults for future contribution on the same page.

The form for assigning contribution credit changed recently, and we're all still getting used to the new process. Thanks again for contributing.

I have reviewed the changes, and everything looks spot on.

I've run the tests locally, and carried out some manual testing, and everything is working as expected.

Moving to RTBC.

@gaurav gupta, contribution credit currently assigned to you, but if you are contributing on behalf of an organisation, you should check the contribution credit and add your organisation(s). You can also click "volunteering my time" if you are contributing some or all of your time on a voluntary basis.

You can set defaults on this page so that future contribution will be attributed to your organisation.

Merge completed. Closing issue.

Thank you for your work on this, @gaurav gupta. Contribution credit should now be assigned.

Don't worry, not forgotten, was just waiting for the pipelines to complete before closing.

Thank you, I have reviewed the change, and everything looks good.

I have tested manually to confirm it's behaving as expected.

I was unsure whether the "save global settings" button should be in the fieldset or at the bottom of the page, but I've had a look at the core "search pages" module, which is the closest example I've found in core, and it puts the save button at the bottom of the page, which is consistent with this change.

Thanks you for making the late nit-picky change. I've since realised that this function will be removed by 📌 Add autowiring to modules controllers and forms Active so it probably wasn't that important.

Moving to RTBC.

Just one tiny fix, then should be ready to merge.

Thanks for your work on this!

I would like to opt-in this module: https://www.drupal.org/project/navigation_extra_tools →
It has 32 issues, and all maintainers agree we are ready to try GitLab issues. We understand we cannot revert this change and there may be some unexpected issues as early adopters.

I did some manual testing, and unfortunately there is still a small problem.

The "Clear storage" option is a child of the "Project Browser" submenu. However, that options has a permission requirement of 'access administration pages'. I think that should be changed to 'access navigation extra tools cache flushing'.

lostcarpark → created an issue.

lostcarpark → created an issue.

lostcarpark → created an issue.

I agree this makes sense!

Currently tests are failing. It needs a very small change to the setUp function of NavigationExtraToolsProjectBrowserTest to fix. Just needs the user to be created with the correct permission.

Update to specify correct form.

I'm very sorry, I made a mistake! I didn't look closely enough when opening this issue. I shouldn't do this when I'm tired!

The SettingsForm isn't used at all. I'll create a follow-on issue to delete from the project.

You've correctly identified that VerifyEmailListBuilder is where the setting should go.

The Secret Length value should go where the "placeholder" markup field is currently.

I will update the issue.

lostcarpark → created an issue.

I have reviewed the changes in this MR.

The changes are only to PHPStan ignore rules, to cause specific errors to be ignored, specifically to skip known safe deprecation warnings, and for Drupal 10, to ignore warnings about hook attributes.

I haven't done any manual testing, but as there is no change to the module code, there should be no change to module behaviour, so manual testing should not be necessary to validate it.

I am happy that this change will correctly suppress hook messages for Drupal 10 only, and will suppress deprecation warnings appropriately for each version.

Moving to RTBC.