Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Ronseal issue.
Comment | File | Size | Author |
---|---|---|---|
#47 | 1947720-47.patch | 168.82 KB | damiankloip |
#41 | 1947720-41.patch | 183.47 KB | damiankloip |
#39 | 1947720-39.patch | 187.07 KB | damiankloip |
#37 | 1947720-37.patch | 187.06 KB | damiankloip |
#33 | 1947720-33.patch | 185.87 KB | damiankloip |
Comments
Comment #1
BerdirCross-reference with #1937600: Determine what services to register in the new Drupal class
Comment #2
damiankloip CreditAttribution: damiankloip commentedRolling now
Comment #3
damiankloip CreditAttribution: damiankloip commentedChange of plan, let's postpone on the refrenced issue in #1
Comment #4
damiankloip CreditAttribution: damiankloip commentedComment #5
damiankloip CreditAttribution: damiankloip commentedComment #6
BerdirIt's not necessary to use \ in non-namespaced code.
I don't think this should talk about the function/method at all, it should IMHO just be "state system". Neither the function nor the method is a system, whatever's behind is.
;)
Comment #7
xjm#6 is exactly why we should split up the
config()
patch. :)Comment #8
damiankloip CreditAttribution: damiankloip commentedSorry, that was silly, just use a regex to do the replacement! :)
xjm, not sure what you mean, but yes, splitting things up is good. one issue per Drupal helper function.
Comment #9
Berdir@damiankloip: That was in reference to our discussion in #1957142: Replace config() with Drupal::config(). We agree that it should be at least an issue per function but @xjm is suggesting to split it up even more.
And, I'm not convinced by that by #6 :) Most of the things that I pointed out would have been syntax errors and changes in .js files are also obviously unrelated. Anyway, let's get the main issue in and then we'll see how easy this one will go on for example :)
Comment #10
xjmlol @ 84K interdiff
I'll stop abusing this issue and discuss over in #1957142: Replace config() with Drupal::config()...
Comment #11
xjmWell, this patch should either be broken up, or use a better regex. ;)
Comment #12
damiankloip CreditAttribution: damiankloip commentedI don't think you can treat my lazy patch rolling as a good reason for splitting this into lots of issues. I will move that dicussion over there though :)
Comment #13
damiankloip CreditAttribution: damiankloip commentedComment #15
BerdirAh, looks like DUBT is missing the definition for the state service and it worked so far because state() still accessed the keyvalue service. My state cache issue has the fix for this too.
Comment #16
damiankloip CreditAttribution: damiankloip commentedYep, you're right :) This should fix it...
Comment #18
damiankloip CreditAttribution: damiankloip commented#16: 1947720-16.patch queued for re-testing.
Comment #20
damiankloip CreditAttribution: damiankloip commentedNot sure what is going on here, locally it is fine.
Comment #21
BerdirI can reproduce this with run-tests.sh, haven't looked into it.
Comment #22
damiankloip CreditAttribution: damiankloip commentedJust a reroll.
Comment #24
damiankloip CreditAttribution: damiankloip commentedThere was a state() call left from the field entity patch that went in.
I think we still have the AddFeedTest issue though, not sure what is going on with that still...
Comment #26
damiankloip CreditAttribution: damiankloip commentedLet's reroll, and see if it's still failing.
Comment #28
damiankloip CreditAttribution: damiankloip commentedI'll reroll, that was just a git reroll. There are a few usages of state() being used since this patch was last properly rolled.
Comment #29
damiankloip CreditAttribution: damiankloip commentedRerolled, still not sure wtf is going on with the drupal_add_feed tests. Something on webTestBase?
Comment #31
BerdirThis is why :)
Comment #32
BerdirThis is why :)
Comment #33
damiankloip CreditAttribution: damiankloip commentedHaha, what a great interdiff.
Comment #34
Berdir.
Comment #35
Berdir... double post.
Comment #37
damiankloip CreditAttribution: damiankloip commentedRerolled
Comment #39
damiankloip CreditAttribution: damiankloip commentedPlease!
Comment #41
damiankloip CreditAttribution: damiankloip commentedSome leftover views changes crept in. This SHOULD be good.
Comment #43
Berdir#41: 1947720-41.patch queued for re-testing.
Comment #44
tstoecklerYup. Let's get this in. Almost 200K aren't that cool to review...
Comment #45
catch#41: 1947720-41.patch queued for re-testing.
Comment #47
damiankloip CreditAttribution: damiankloip commentedRerolled. Can you we this is asap please - It's a pain to reroll!
Comment #48
catchPutting this back into the RTBC queue so it's ready to go once the bot comes back green.
Comment #49
damiankloip CreditAttribution: damiankloip commentedGreat- thank you!
Comment #50
catchCommitted/pushed to 8.x, thanks!