Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Comment | File | Size | Author |
---|
Issue fork drupal-3112295
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:
Comments
Comment #2
mpdonadioOK, took the patch from 2902895-43.patch
- Isolated just the OO code (well, I tried); this includes classes w/ and w/o access to the container as a starting point
At this point will start going through `git diff --name-only 8.9.x` to isolate by path/namespace revert changes to classes that def have container access
Comment #3
mpdonadioOK, I think these are all of the usages of classes that need to use the \Drupal singleton.
Comment #8
mpdonadioCommit credits.
Comment #9
daffie CreditAttribution: daffie commentedAll code changes look good to me.
The patch is for me RTBC.
There is one small documentation nitpick that can be changed on commit.
Documentation lines are not filled out to the max. Can be changed on commit.
Comment #10
alexpottIt feels odd given that we're replacing REQUEST_TIME to not also replace time() in these three instances with the \Drupal::time() equivalent.
Comment #11
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 for Drupal India Association commentedKindly review a new patch, where time() is replaced with \Drupal::time()->getCurrentTime().
Comment #12
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 for Drupal India Association commentedComment #13
daffie CreditAttribution: daffie commentedThe remark from @alexpott from comment #10 has been addressed.
All the code changes look good to me.
Back to RTBC.
Comment #14
alexpottCommitted d23b4cc and pushed to 9.0.x. Thanks!
We could have a follow-up to simplify this a bit. The whole point is to generate a unique random identifier. This feels overly complex. But no need to address here.
@Hardik_Patel_12 it would have been great if you could have addressed #9 in #11 as well.
Fixed on commit.
Comment #17
alexpottNeed to work out why but this patch has caused instability in the test system. Locally I can't get
core/tests/Drupal/KernelTests/Core/Entity/ContentEntityChangedTest.php
to pass. And we're seeing other fails too - https://www.drupal.org/pift-ci-job/1586930Comment #18
alexpottOh fun... so at least the fail in
core/tests/Drupal/KernelTests/Core/Entity/ContentEntityChangedTest.php
happens because in KernelTestBase we do:Because it does
Request::create('/');
this ends up having a different time than $_SERVER['REQUEST_TIME'] (and therefore the legacy REQUEST_TIME) because this request is created using the result oftime()
not the server globals.I think this means efforts to do conversions bit by bit are fraught and that we might need to do everything at once.
Changing
Request::create('/');
toRequest::createFromGlobals();
fixes the test but I don't think this change is correct.Comment #20
Kristen PolSee possibly related issues noted here:
https://www.drupal.org/project/drupal/issues/3112283#comment-13605217
Comment #24
andypostshould move request time out of loop
Comment #25
mondrakeComment #26
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAdded reroll of patch #11 on Drupal 9.4.x.
Comment #30
AkashKumar07 CreditAttribution: AkashKumar07 at TO THE NEW commentedAdded reroll for 9.5.x-dev version.
Comment #31
mondrakeThis impacts PHPStan baseline so better target 10.1.x first. Changing to MR worflow.
Comment #32
mondrakeCan't create a MR against 10.1.x for some reasons, fallback to 10.0.x
Comment #34
mondrakeComment #35
mondrakeAddressed #24.
Comment #36
mondrakeAddressed #9 plus another crufty comment.
Comment #37
smustgrave CreditAttribution: smustgrave commentedBelieve this missed the 10.0 boat but can land in 10.1 hopefully.
Moving to NW for the MR to be updated to point to 10.1 please
Comment #39
mondrakeI closed the MR2467 to avoid becoming a bottleneck. Reverting to patch workflow, rerolled patch.
Comment #40
smustgrave CreditAttribution: smustgrave commentedNot sure why but when I tried to run ./core/scripts/dev/commit-code-check.sh to check this patch it now locks up.
Comment #41
andypostcould use variable to prevent calling twice if both are 0
Could use variable $time = \Drupal::time()
Comment #42
andypostBtw is there a reason to replace
time()
with service call? I think only for unit-testing reasonsComment #43
smustgrave CreditAttribution: smustgrave commentedAddressed points in #41
For #42 I believe it was made for cleaner code? But can't be sure.
Also when I try to run the code check it locks up. Is that normal when making changes to phpstan files?
Comment #44
DanielVezaThis needs a reroll for 10.1.x, patch no longer applies.
Ran the test fails on the latest 10.1.x with this patch with --repeat=20.
SaveActionTest
passed, but EntityValidationTest is now flaky. It fails about 30% of the timeComment #45
_pratik_ CreditAttribution: _pratik_ as a volunteer and at Specbee for Drupal India Association commented#43 patch not applied for me also, Please try this new patch.
Thanks
Comment #50
acbramley CreditAttribution: acbramley at PreviousNext commentedMoving back to an MR workflow, rebased #45 on to a new branch off 11.x in a new MR
Comment #53
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedThe MR is green but we did have another failure earlier which I couldn't reproduce locally https://git.drupalcode.org/issue/drupal-3112295/-/pipelines/85141/test_r...
I'm not too sure how this could happen when we're sleeping
Comment #54
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedAnother similar random failure this time in testEntityChangedConstraintOnConcurrentMultilingualEditing
Comment #55
Taran2LWas looking at this a bit, and I'm not quite sure some of the examples actually do not have access to the container
Comment #56
Taran2LComment #57
andypostBaseline needs clean-up
Comment #58
Taran2LShould be good to go now
Comment #59
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedFound one instance that can use DI
Comment #60
Taran2LComment #61
Taran2L@acbramley, something like this ?
Comment #62
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedLooks perfect! Nice one.
Comment #65
catchCommitted/pushed to 11.x and cherry-picked to 10.3.x, thanks!
Comment #66
andypostIt was the last in list, so META can be closed as well!
PS: please close MR