No weird changes, actual green tests in main MR and also the test mr with updates package. Nice :)
Updated lock file hash.
Added the requested docs.
Mis understood, RTBC was referring to the change record.
Updated CR
I normally search like this: https://git.drupalcode.org/search?group_id=2&scope=blobs&search=-path%3A...
Since when are we opening issues on contrib projects for deprecations/removal? I've not seen that before :)
Oh, will rebase later today.
Bot is broken
Rebased
Edited because LSEP character in title is no fun to handle when using the API and import the data into a neo4j database.
Still open comments on the MR, those need adressing. :)
All green and happy after the rebase.
Made the few small changes you asked for, setting RTBC, all seem straightforwards enough and tests are still green. Updating Cr right now.
Never mind me, all good, that is the branch which uses 6.x to test. Adding ignore so tests run, but nothing to worry about. Keeping RTBC
Seems we have an issue left here after rebase:
------ -------------------------------------------------------------------------
Line core/tests/Drupal/Tests/Core/Theme/Component/ComponentValidatorTest.php
------ -------------------------------------------------------------------------
351 Call to an undefined static method
JsonSchema\ConstraintError::FORMAT_URL().
🪪 staticMethod.notFound
355 Method JsonSchema\Constraints\BaseConstraint::addError() invoked with
4 parameters, 1-3 required.
🪪 arguments.count
------ -------------------------------------------------------------------------
This is the changelist:
https://git.drupalcode.org/project/coder/-/compare/8.3.28..8.3.29?from_p...
There are changes to that sniff it seems.
I would suspect: https://git.drupalcode.org/project/coder/-/commit/78034f0a41956c4f8d3808...
THis is the changelist:
https://git.drupalcode.org/project/coder/-/compare/8.3.28..8.3.29?from_p...
There are changes to that sniff it seems.
I would suspect: https://git.drupalcode.org/project/coder/-/commit/78034f0a41956c4f8d3808...
Ty and merged. Nobnew release though, do younneed one?
Confused, the validation runs against the constraint, i try to make cases for the constraint that touch the different paths throuugh the constraint.
In the parent issue there were indeed 2 constraints that needed tests, and since these validation issues have been broken down a lot, i opted to create 2 issues for those contraints. But to test, you need the contraint also so it is implemented here.
Now sure what test you are missing, isnt NoEntitiesExistYetWithHigherCardinalityTest a test for this contraint? Seems reasonable to test like that. If you recon that is not enough coverage please let me know what you want more.
Shoudlnt the parameter be typed? Its new and seems to be an ?orderinterface
Tags made here 📌 New Drupal 12 compatibility tags Active and updated the default config in the repository to use those.
New branch has been made master-d12, unit test will probably fail.
Well, tags are made and references in project_analysis.
Ok we need the following tags in core:
ProjectUpdateBotD12, Drupal 12 compatibility, ProjectUpdateBotD12
I'll make those.
Ah thanks. Changes look good as i said earlier.
Looking at the other issue, it seems we might have already made this with this cycle in mind, and we just need adjusting project_analysis to filter out 11.x compatibilty when parsing all the updates.xml.
Moving to Drupal.org issue queue. Seems better.
Failure is unrelated, yay for build errors. Changes look good, al comply with the description of the variable. Its not NR, so cant do much more than this?
quietone → credited bbrala → .
Found the earlier issue i think 📌 Update SQL query to generate project list for project_analysis and deprecation_status Fixed which was against drupalorg?
No i am not, removed you from assigned since i saw no activity, but do on ahead! :)
https://git.drupalcode.org/project/drupal/-/merge_requests/4814#note_403040
That one is unresolved also, I was not confident enough to close that one :)
That rule removes when there is no description, so its partial
A quick scan of core test doesn't reveal any other places where file_put_contents is used that does not use a site scoped directory or tempnam/randomMachineName to generate an unique file name.
Wow lovely, wonder if there is more places where we hard code write to a temp file. If we do need to write we should probably use https://www.php.net/manual/en/function.tmpfile.php to create a unique handle and cleanup.
For this test the fix makes sense to just do it staticly.
2 unresolved threads that need attending to.
Created a child: 📌 Remove @var for strictly typed class properties Active
Probably not, just make one?
3.29 was released should we use that?
Merged these changes into the branch I detected the failure in earlier today. Checked if i still got the failure as i did then. I didnt, so I am pretty sure we are all good now. ;)
Rebased, all green. yay.
For the record. I'd pick:
"Unit tests: Core (PHP 8.3)"
"Unit tests: Components (PHP 8.3)"
Head fixed, still a test failing, but it seems so unrelated...
Maybe we can exclude this file from phpstan?
Head is broken on phpstan.
Fixed the tet error. But getting phpstan errors on function t(...) which is weird, since it is fine o_O
Tried rebasing, updating 11.x in fork, not trying empty commit. Weird...
Looking good, left some small comments.
Went through the changes, these look good.
For a small moment i thought, why wouldnt we not just update the baseline? But that might be a hassle when switching between 5 and 6.
And in regards to contrib, it is pretty easy to support 5 and 6 for now, change is not that bad as you can see in this mr.
Anyways, all good, cool ^^
Not sure why we would remove "Unit tests" from the title here, i understand it's now in a "Unit test" stage. But lets consider;
- Running locally would mean running "Core on PHP 8.3"
- If tests fail "Core on PHP 8.3" is the title of the job.
- All other jobs are things like "PHPUnit Functional"
I'd probably opt to call them something like:
"Core Unit tests on PHP 8.3"
"Component Unit tests on PHP 8.3"
Or maybe:
"PHPUnit Unit Core (PHP 8.3)"
"PHPUnit Unit Components (PHP 8.3)"
"Unit tests: Core (PHP 8.3)"
"Unit tests: Components (PHP 8.3)"
Know this is pretty nitpicky, but we are gonna look at those a lot :)
Agree with Andy. Might even advocate stop using child pipelines all together.
Current changes look clean and nothing stands out as weird.
If we can remove more ignores we could, but might as well be in a followup.
json-schema has a BC break also on addError, which seems like it will be fun.
Seems it has an undocumented change:
https://github.com/jsonrainbow/json-schema/issues/822
BaseContraint:
public function addError(?JsonPointer $path, $message, $constraint = '', ?array $more = null)
to
public function addError(ConstraintError $constraint, ?JsonPointer $path = null, array $more = []): void
Rebased.
https://github.com/micheh/phpcs-gitlab/releases/tag/2.0.0
https://github.com/micheh/phpcs-gitlab/compare/1.1.0...2.0.0
phpcs-gitlab is not really that big a change. Mostly changing how to hash/track issues, which prompted a 2.0.0 release.
Added tests for different situations and verfied install_proffile parameter is all types we discussed. Unfortunately i couldnt get unit tests working, so ended up added an extra method to disable the fact it is inside a test.
Was going to ask why we are not calling this Depth instead of MaxLinkDepth and coupling to the menu. But seems getting maxDepth in some other way it rather hard. So guess my thoughts are a no-op.
Just want to note; we do have AtLeastOneOfValidator nowadays, although cleaning up is good, just want to. mention it.
Removing tag since committer who will review this seems to be the subsystem maintainer.
All green and happy. Seems so help at least a bit. Let's do it since impact should be low.
review bot is right, we need FASTZIP either ignored or added to the dictionary.
Updated to propere extension discovery.
Had to add a testing env check because there are different rules there what modules you can install :(
All good now i think (and green tests)
Ok fair enough.
This signals the test is trying to set a module from outside its installed profile. Should be a weirdness with the teatsetup as talked about with alex
Those services were a little involved to instantiate. Pulling in a lot of stuff from the container. I could then remove the test changes, but i did need to update a theme test that was loading a module from a profile that is not installed.
Think this is working as expected now thanks to the guidance of alex.
Thanks!
Left a comment, also phpcs is not passing :)
If this works, i think i will split it into a seperate constraint maybe. Since then we could just instantiate the new ExtentionList classes every time instead of doing the check. Which would also mean i can just inject the services in the create method with having loads of extra services in the ExtensionAvailableValidator
Lets see how this works when we forcefully reinstantiate the discovery mechanisms.
Form slack
alexpott
16 minutes ago
Yeah but it doesn’t work
4:53
The problem is profile supplied modules
4:53
Changing a profile changes which directories module and theme discovery looks in (edited)
bbrala
15 minutes ago
Oh yeah
alexpott
15 minutes ago
The install profile setting is a fundemental setting that is extremely powerful.
bbrala
14 minutes ago
Grmbl, ok back to the drawing board.
This wouls fail when there is an eztention that would only become available when the profile is loaded. Right?
alexpott
14 minutes ago
Yes
4:56
We need to detect profile change and then use fresh extension discovery objects with the correct profile directory in the constructor. In this case we can not use the discovery from the container because we need to look at what will happen when the install profile is changing.
4:57
Also just for fun you’ll have to cope with the case when the install profile value is being unset. Because this is actually the only change possible via the UI or API.
4:58
And by API I mean the module install API - the direct config API should not really be used for core.extension.
bbrala
10 minutes ago
Guess i'll start designing 2 tests for that then. Should be able to reuse some other profile for that.
alexpott
7 minutes ago
Well this test is a test of sorts… testing is hard because we also allow stuff in test land that’s not normally allowed… see \Drupal\Tests\drupal_system_listing_compatible_test\Kernel\SystemListingCrossProfileCompatibleTest
bbrala
6 minutes ago
Hmm ok
5:05
Ok, then i'll just go back to my first idea, and indeed find out how to reload/reinstantiate the *ExtentionList with the changed profile if applicable and validate against that.
5:06
New root, and no cache i'd assume.
5:08
and install profile it seems
alexpott
1 minute ago
I think that that is way to go. I don’t think relaod/reinstantiate… just instantiate… see \Drupal\Core\Extension\InstallProfileUninstallValidator::getExtensionDiscovery
5:08
I think using file cache / info parser is fine tbh.
bbrala
Just now
thanks, that helps. Feels like it is possible :slightly_smiling_face:
Think i might've found a way by adding special case for core.extension. Lets see how tests go.
"I'm not sure. I guess we can leave it alone until it causes us problems."
Not sure what you mean with leaving it alone. Ignore the weird test case where profile is changed and module is changed?
Lets start by saying, i dont know much about the systems in play here ;)
Hmm, so my first thought would be that it should do discovery without any cache, but ending up at that not being really a thing. Lets see if i understand correctly.
We need to validate the extention, BUT, since profile is also changed it might have different modules available. So even if we say; when the profile changes, we reload the list or rescan for available modules. BUT this validation is before the changes have persisted. So it gets worse, in the fact that it needs to scan for the future situation.
In my head i have a few options;
- Make an extended version of this validator that also checks for value of profile, use that to check if the value has changed, if so, do a clean discovery of the available modules and use that for validation.
- Use your suggestion that allows for modules that are also profile to be installed. But that could lead to trying to install a module that is name as a profile efven though it doesnt exist?
- Don't allow profile changes, but that seems kinda hard.
Fixed issues, but kinda need someone into validation to review this since it feels weird this works at face value. It might overlook issues.
Wouldn't this be solved here: 🐛 olivero.settings config schema is wrong Active ?
I rebased
Think i addressed all feedback
Double checked, there is no code dividing, so guess, 0+ is fine then.
Not sure if 0 is a good idea. It might be possible, but could also end up devision by zero. Also don't see the value in adding 0 as an option. You sure you want 0? 1+ seems safer and make more sense.
What the... with a little change in our validator we would be able to use these easily? What am i missing....
Wow, am i dreaming. This little trick might make it possible to support those symfony constraints without weirdness.
Everything is green... It cant be that easy right?