The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.Problem/Motivation
Now that almost all hooks can be OOP, .module files are only necessary when/ if they contain preprocess functions
Steps to reproduce
Open file
Proposed resolution
- Move functions from the ajax_test_forms.module file into the Drupal\ajax_forms_test\Callbacks class.
- Add a return type to the moved functions.
As part of this issue, we are going to convert all methods at Drupal\ajax_forms_test\Callbacks to static methods.
Remaining tasks
None
User interface changes
N/A
Introduced terminology
N/A
API changes
N/A
Data model changes
N/A
Release notes snippet
N/A
Issue fork drupal-3495959
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
nikolay shapovalov commentedComment #4
nikolay shapovalov commentedReady for review.
Comment #5
smustgrave commentedIt's a test but wonder if we should start using a Callback folder?
Comment #6
smustgrave commentedMaybe a conversation for another issue. Since there are so many modules in system I just all the ones that started with ajax and these were the ones that needed conversion.
Comment #7
larowlanAny reason why these can't be static methods?
Comment #8
nikolay shapovalov commentedYou are right.
It looks like most of the callbacks methods should be converted to static.
Comment #9
nikolay shapovalov commentedExisting methods at Callbacks are not static, this is the reason I decide to use non static methods, to keep code consistent, I think we can create follow up to convert methods to static and add return type hint. I revert recent changes I made. And set back to NR.
Comment #10
smustgrave commentedSince we are dealing with just tests that think it could be in scope of during the conversion to make static here vs another issue.
Comment #11
nikolay shapovalov commentedAll methods at Drupal\ajax_forms_test\Callbacks converted to static and added return type hints. MR is ready for review.
Comment #12
smustgrave commentedThanks, looks good to me.
Comment #14
catchOnly used internally to the test module so we don't need to worry about a bc layer. Approach looks fine to me. Committed/pushed to 11.x, thanks!
Comment #17
catchLooks like this broke HEAD:https://git.drupalcode.org/project/drupal/-/jobs/4115231
Reverted for now.
Comment #22
nicxvan commentedI don't think I have permission to reopen the MR so I pulled it down, rebased on 11.x and pushed up a copy so I could create the MR.
Since I was doing that I also reviewed the MR. If it's green I think I can RTBC since the rebase was clean and I didn't write this one.
Comment #23
nicxvan commentedComment #24
quietone commented@nicxvan, just ask in #core-development for help re-opening an MR. Someone is often around.
Why did this break HEAD and has it been fixed?
Comment #25
catch@quietone not sure but it may have just been a cross-commit with something else where they passed individually and failed when combined. We have a lot of 1+ month MRs in the RTBC queue so the chances of that happening are quite high. It happened with at least one other issue too. When we're using gitlab issues, we'll be able to set up merge trains so that each merge gets a full pipeline run before it actually goes in against the current state of HEAD.
Since the rebase is green, going to try this again but also keep an eye on gitlab in case we need to revert again.
Comment #28
godotislateHEAD seems to be broken again with 38de8a6c.
Comment #29
nicxvan commentedWhere are you seeing that?
Is it a new random fail?
Comment #31
godotislateIn my MR for another issue: https://git.drupalcode.org/issue/drupal-3416819/-/pipelines/405059/test_...
Also testing locally latest 11.x fails. I have pushed up MR with the fix, but it's concerning that tests were passing earlier.
ETA. Not sure if it's related to #3494332: Functional Javascript tests are silently skipped on Gitlab CI due to selenium standalone configuration
Comment #32
godotislatePushed up MR 11001 with the fix.
The test only job is passing erroneously, which is probably #3494332: Functional Javascript tests are silently skipped on Gitlab CI due to selenium standalone configuration.Never mind - test only job includes the test fix, so that's why it's passing.
Comment #33
oily commentedComment #34
nicxvan commentedOh good catch, why was it passing without it though?
Comment #35
godotislateYes, it's very concerning if functional javascript tests have false negatives on MR builds.
Comment #36
oily commented@godotislate I experienced this on another issue and @berdir had a solution.
Comment #37
nicxvan commentedI did another careful review of the merged MR and confirmed that this was incorrectly converted, and that MR 11001 fixes that. I also confirmed that no other functions were incorrectly mapped to their corresponding methods.
RTBC
Also @quietone, I'll keep that in mind in the future! Honestly it really was no trouble, it's just a couple of extra commands to create a new branch and push it up.
Comment #38
nikolay shapovalov commentedMR 11001 looks like good fix. @godotislate thanks.
+1 for RTBC
I think this false positive. Tests passes but it should fail.
@nicxvan thanks for double check original MR 10676, and also found nothing else, but this.
Comment #41
catchBumped #3494332: Functional Javascript tests are silently skipped on Gitlab CI due to selenium standalone configuration to critical, tests were passing multiple times both on here and branch runs when they should have failed.
Committed/pushed the latest MR to 11.x, thanks for tracking it down.
Comment #42
nicxvan commentedThanks!
Godotislate should be credited I think though.
Comment #43
catchAhh sorry too many things, also didn't mark it fixed...
Comment #45
nicxvan commented