Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
Comment | File | Size | Author |
---|---|---|---|
#27 | interdiff-2848812-24-27.txt | 657 bytes | yogeshmpawar |
#27 | 2848812-27.patch | 5.08 KB | yogeshmpawar |
#24 | 2848812-24.patch | 5.04 KB | harsha012 |
#22 | 2848812-22.patch | 4.9 KB | harsha012 |
#15 | 2848812-9-15-interdiff.txt | 3.03 KB | Sharique |
Comments
Comment #2
pritamprasun CreditAttribution: pritamprasun at OpenSense Labs commentedComment #3
pritamprasun CreditAttribution: pritamprasun at OpenSense Labs commentedComment #4
JayKandariComment #5
cilefen CreditAttribution: cilefen commentedComment #6
JayKandari#2 looks good.
another instance of "db_set_active()" is present in following File: Kindly add this to #2 as well.
core/lib/Drupal/Core/Database/Install/Tasks.php:159
Thanks!
Comment #7
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedcall to db_set_active in core/lib/Drupal/Core/Database/Install/Tasks.php:159 also changed with new syntax.
Comment #8
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedComment #9
JayKandariMinor Code standards issue. Else it looks good.
Minor correction :
\Drupal\Core... lines are having an extra space before the code.
Kindly correct these as well. Thanks !!
Comment #10
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedLine spaces corrected.
Comment #11
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedComment #12
JayKandariPatch #10 Looks good.
Thanks !
Comment #13
cilefen CreditAttribution: cilefen commentedThe meta issue reads: "...replace all usages of the function (except tests for the function itself)...". LoggingTest is the only test calling db_set_active(), it's not exactly specifically testing the global function but as it's the only test, I'm not sure what we want.
Comment #14
xjmComment #15
Sharique CreditAttribution: Sharique as a volunteer commentedIn my opinion we should replace LoggingTest instance also.
Replace full namespace call, as namespace is already in use statement.
Comment #18
volegerPatch #15 still applying. Tested on 8.6.x.
Looks good
Comment #19
alexpottThis patch should add an
@trigger_error()
to db_set_active() so people can discover they need to replace the calls. See https://www.drupal.org/core/deprecationThis will also prove we've replaced all the usages in core.
Comment #20
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commentedfixed as per #19
Comment #21
alexpottThe message should all be on one line otherwise it won't flow right when displayed in logs etc...
Also it would nice to add a test of the db_set_active() function to \Drupal\KernelTests\Core\Database\RegressionTest(). You'll need to add
@group legacy
and@expectedDeprecation deprecation message
to the new test.Comment #22
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commentedchange recored - https://www.drupal.org/node/2944084
fixed as per #21
Comment #23
alexpottThis should end with ...
instead. See https://www.drupal.org/node/2944084.
The docs are
So I can't see how this works.
Comment #24
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commentedimproved the test
Comment #25
alexpottThis method returns the key. Let's assert against that rather than truthiness.
Comment #26
yogeshmpawarComment #27
yogeshmpawarAddressed the #25 comment on updated patch & also added an interdiff.
Comment #28
volegerLooks good for me
Comment #30
catchCommitted/pushed to 8.6.x, thanks!
Comment #31
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedGreat!!. I hope other issues like these will be pushed as well.
Comment #32
voleger#31 +1
There are alot issues like this (db_* replacements) that should be synchronized after each commit.
So If some of the other issues are RTBC it would be great to review and commit to saving the time to track changes at the related issues.
Anyway, thanks)