Good find! Also found a few other issues related to the last release on D11, D10 and d9 compatibility, so committed those too.
japerry → made their first commit to this issue’s fork.
Yah, this looks correct. Fixed!
Made comments on the MR -- there are BC issues and I think we can use reflection instead of needing another public method.
Correct, I'm not too worried about those tests failing for now -- but when commerce has a release they should automatically work here. If not, we can follow up with fixes in the tests or code if needed.
RC3 released with D11 support. Won't be doing the other issues in this plan before 4.0, but they can come in a later point release or in 5.0, pending some changes with ctools
RC3 brings support for D11 and closer support for newer versions of ctools. Please give it a try, if there are bugs I'll try to fast-follow. Tests are passing, except for the schema issues, which would be nice to get in before a full release.
After doing some manual tests, it doesn't appear to be breaking things for me... so Fixed!
Hmm... I'm not quite sure, the D11 tests are still failing the schema check, even with Patch #8 being committed to head. Updating MR so we can see tests running.
Bleh this is why I don't like linting. Reverted that change and committed to head.
Thanks for the tip! Sent.
japerry → made their first commit to this issue’s fork.
Thanks @vishalkhode! However, there are still remaining tests that are failing due to other modules not being D11 compatible. If there are things we need to fix after those modules get D11 support, we can re-visit it then.
for now I think we're good to commit this. Fixed!
japerry → changed the visibility of the branch 2914963-explicit-declaration-of to hidden.
I did commit this because it didn't break any of schemata's tests. Thats not to say it won't break other tests. Before release, it might be good to have openapi_jsonapi and hal tests against dev.
The code in the commit will break pre Drupal 10.2, and isn't required until Drupal 12. Should wait until gitlabci is building properly before this gets committed.
This might be a good candidate for the Project Update Working group. We can expedite the fixes without needing to add maintainers.
I do have concerns however that this issue is smashing different issues (phpcs, phpstan, cspell, etc) with D11 compatibility. Especially since tests are failing now due to some method signature issues (that need checking to make sure BC works with D9.5.
Thanks! makes it easier for me to review other code :-D
Pushed an MR that fixes some underlying Interface definitions and adds a fallback if getThirdPartySetting is missing.
While the tests are failing, these are related to group and commerce. Not sure what, if anything should be done with those as the module appears to work fine outside the tests in D9.5, 10, and 11. Fixed!
I think I found a happy medium. By adding 'modules' to the DRUPAL_PROJECT_PATH, it allows the system to default to modules, but provides themes and profiles the ability to change the variable to what works for them. In a perfect world I still think moving the project under test to its own folder and using composer is better, but that should be moved to another issue (despite me originally arguing the opposite).
Hopefully this latest round of changes is more acceptable. It allows for BC, it also fixes problems for themes/profiles, and allows modules to move into the more 'standard' location expected during tests.
Went down a rabbit hole trying to fix the themes/profiles symlink mess. My latest update to the MR puts these in the correct place, but some of the scripts still need updating.
There is now question as to whether this is a breaking change or not -- themes and profiles are symlinked as modules, which is not intentional and likely cause issues for these types of projects. Using the built in composer symlink functionality and moving the project under test to its own folder fixes some of the oddities the existing build system has, including its bias towards modules.
Added the variable. Note, its just the custom/contrib switch because themes, profiles, and modules should be observing this pattern, and having a matrix of these variables for D7 and D8+ doesn't make much sense.
We wouldn't even need to have "contrib" or "custom". So I'd really say that this is kind of working as designed and it's more of a feature request.
I think a module should work regardless of whether it's inside a "contrib" or "custom" folder, as Drupal has some methods to get the path of the module.
While technically true, the recommended/supported way is for community modules to exist in contrib and site specific modules to exist in custom. Many templates, including the official drupal recommended project organize things this way, so I don't think our tests should be deviating from that.
The reason why custom vs contrib is important is that you may use .gitignore on the 'contrib' folder, and in our case, differentiate between 'community' and 'site specific' code when running automated scans. If we cannot use the standard setup, our tests fail.
In general I’m contending once you patch/modify/change the code of the official module it can be perceived as a custom module (an uncommitted MR is custom code).
Thats somewhat antithetical to the whole concept of contrib testing. The whole goal is for an MR to become part of the official code, and should be tested as such. Otherwise, why make the issue in the first place? It also isn't true--the official and recommended way modules are to be installed is via composer, which uses the scaffold, which recommends using contrib. If you patch a contrib module it will still be in the contrib folder because its being updated by composer, not being committed directly to the site's folder, as you'd have with custom. While its not released yet, core will be making contrib essentially a requirement as part of composer ready templates.
That all said, I'd be all for making the module path a variable so the 1.x version can remain the way it is while 2.x has this fixed.
The settings parameter was removed at some point from the backend constructor, so this shouldn't be an issue anymore
While investigating this further, I noticed we are using config instead of settings.php for the driver settings. That seems wrong. I've made a change record and will note it in the 2.6 release notes. https://www.drupal.org/node/3460878 →
japerry → made their first commit to this issue’s fork.
Tests are passing and reliably both locally and on gitlabci! Thanks Wim for the rubber ducking with me!
Tests need to be rewritten as dataProviders must be static, making the design for test doubles not workable.
https://stackoverflow.com/questions/76853088/how-to-create-test-doubles-...
I would look at the dev version, I think I fixed this issue yesterday.
japerry → made their first commit to this issue’s fork.
I.. think this is a duplicate of 📌 Modify form structure and use core 'states' JS for each access plugin Fixed which was committed?
I believe this is fixed with a5aee306 -- please verify with the latest dev (or release upcoming
After cursory review, I believe there are more translations that are missing. But I agree we need to get this fixed, marking Major because its actually making review of tests difficult.
Agreed, this is a better approach and isn't causing permission saves to fail. Committed.
Much cleaner approach! Committed.
japerry → made their first commit to this issue’s fork.
This should have a test before we commit it.
This was fixed in a5aee306
I would opine that the GitlabCi template aligns with this as the code being tested is not guaranteed to be in the official repo and as such is closer to a 'custom' module.
I don't quite follow here -- Drupalci on gitlab is specifically for testing code being hosted on Drupal.org. While the specific MR under test may not be guaranteed to be committed, the module/theme/etc which is receiving the MR is, by definition, not a custom module.
Modules/themes/etc hosted on Drupal.org expect to be installed in the contrib folder, not the custom folder, and thus the test system is not performing an accurate test. Modules that need to differentiate between contrib and custom modules will fail tests when using gitlabci.
As for a 2.x version -- While thats technically correct (due to .composer-base changing), I haven't ran into a case where paths are being manipulated after a composer build. Certainly possible though. However, this issue is currently breaking our tests, so we are depending on this fork/ref until its adopted.
Probably should wait for the next release of externalauth and up the minimum requirement to that release. That way if you see this error and update samlauth, it'll update externalauth too.
Merged! Could make a release, but it'd be nice if external auth was updated first before a release was made here.