I've reviewed the code and done some manual testing. Everything looks good to me. Nice to see it in ECA!
Moving to RTBC.
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).
Very good work! I have made a few small suggestions. We should then be ready to merge.
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.
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.
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.
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.
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.
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.
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.
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'.
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.
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.
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.
I have tested manually, and ran the automated tests locally. All tests pass and all looks good to me.
Moving to RTBC.
I attempted to fix phpstan (previous major) with a sed script. I eventually overcame the issues with my script and got it removing attributes correctly, however there were some errors not related to attributes. Agreed with @dww that we should revert from this change and move into a follow-on issue.
lostcarpark → made their first commit to this issue’s fork.
Thank you for working on this.
I think the test is pretty neat. It's nice that you were able to make it a kernel test rather than a Functional one.
I've run the automated tests manually, and run the automated tests on my local.
Moving to RTBC.
I have run the tests manually and manually tested, and I'm happy with this change.
I will open a follow-on issue to add a hook to delete the config.
I think it's confusing that relative dates work on Datetime fields, but not on Timestamps, so it would be great to get this over the line.
I assumed that with no dependencies it would automatically install, but I see that's not happening.
I think that moving to config/install is the best solution. However, I think that will fail if the config is already present.
I think we should add an hook_uninstall to remove the config so that if the module is reinstalled it won't fail. However, I think this can be done in a follow on issue.
Apologies.
I realise that starting a test in this change will offer several advantages.
- It will let us use "Test only changes" to verify the test fails without the change.
- It sets us up for extending the test in future changes.
Apologies to add to the requirements mid issue, but I think the test is quite small, and adding it in the issue has enough benefits to justify.
Thank you for your work on this.
Some excellent ideas here.
I think the reason for making classes final might have been to stop PHPStan warning about "unsafe use of new static()" in the create function. I'm aiming to move to autowiring, which should allow most create functions to be deleted. But preventing subclassing is clearly not desired, so let's get the final removed.
Permissions sound like a good way of dealing with redirecting. I could see value in having "allow email verification", and "bypass verification" permissions for each verification form. If you don't want to allow user creation, just set "allow email verification" to "authenticated users". If you want to skip verification for logged in users, then set "bypass verification" to "authenticated users". Otherwise, "bypass verification" could be granted to a role for users who won't have to complete verification.
Another useful option would be to add an option to the verify email form settings to allow a user to be added to a role on successful verification. That way, users could be added to a "verified users" role once verified, and that role could be granted "bypass verification" so they wouldn't be challenged again.
Moving the redirect out of the form does make sense. I have a few thoughts on the best way to do it.
As there are a few different pieces, I'll create some smaller issues to tackle them individually.
Change merged successfully. Thanks for working on this @ritarshi_chakraborty.
Please review the contribution record. If you would like to attribute credit to an organisation, you should select them there. If not, you should tick the "I am volunteering my time" box. You can also set a default for future contribution.
I have reviewed and the change looks perfect. I've also run the tests locally and done some manual testing.
Moving to RTBC.
Thank you for your work on this issue, @nidhi27.
Please review the contribution record. If you would like to attribute credit to an organisation, you should select them there. If not, you should tick the "I am volunteering my time" box. You can also set a default for future contribution.
Thanks for your work on this issue, @souvik_banerjee.
Please review the contribution record. If you would like to attribute credit to an organisation, you should select them there. If not, you should tick the "I am volunteering my time" box. You can also set a default for future contribution.
Merged successfully. Thanks for working on this.
Please review the contribution record. If you would like to attribute credit to an organisation, you should select them there. If not, you should tick the "I am volunteering my time" box. You can also set a default for future contribution.
I have tested, and the config setting was created as expected.
I'm happy to move to RTBC.
Thank you for your work on this.
Everything is technically correct. I would just like the aliases grouped with the service they are aliasing for clarity.
Please reorder, so that the service definition is followed by all aliases of that service. There is no need for a blank line between the service and its aliases, just between each distinct group.
Should be a simple change, then ready to merge.
I fully agree, and I wasn't suggesting that should be a blocker.
Makes sense.
Although, when I install on 8.4, I still get a bunch of deprecation warnings from contrib modules.