Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Comment | File | Size | Author |
---|---|---|---|
#51 | 2848815-51.patch | 4.17 KB | mondrake |
Comment | File | Size | Author |
---|---|---|---|
#51 | 2848815-51.patch | 4.17 KB | mondrake |
Comments
Comment #2
pritamprasun CreditAttribution: pritamprasun at OpenSense Labs commentedComment #3
vidhatanand CreditAttribution: vidhatanand at OpenSense Labs commentedComment #4
pritamprasun CreditAttribution: pritamprasun at OpenSense Labs commentedComment #5
vidhatanand CreditAttribution: vidhatanand at OpenSense Labs commentedComment #6
cilefen CreditAttribution: cilefen commentedWhy is "0" being passed when it was not with db_next_id()?
Comment #7
vidhatanand CreditAttribution: vidhatanand at OpenSense Labs commentedThe db_next_id() has a default argument passed as 0, which probably needs to be passed here now explictly. https://api.drupal.org/api/drupal/core!includes!database.inc/function/db...
Comment #8
cilefen CreditAttribution: cilefen commentedWhy?
Comment #9
vidhatanand CreditAttribution: vidhatanand at OpenSense Labs commentedGot the point, it's actually not needed.
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Database%... too has a $existing_id = 0
Updated patch attached.
Comment #11
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commented.
Comment #12
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedComment #13
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedLets test this patch.
Comment #14
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedComment #15
JayKandariPatch #13 Looks good to me. Thanks !!
Comment #16
JayKandariComment #17
cilefen CreditAttribution: cilefen commentedSee #2232391: Revert drupal 8 changes made to generate-d7-content.sh and generate-d6-content.sh. This file should not be changed.
Comment #18
cilefen CreditAttribution: cilefen commentedComment #19
xjmComment #20
Sharique CreditAttribution: Sharique as a volunteer commentedUpdated patch.
Comment #21
akashkrishnan01 CreditAttribution: akashkrishnan01 commentedDid a quick test, above patch is getting applied successfully
Comment #23
jeetendrakumar CreditAttribution: jeetendrakumar as a volunteer and at HyTechPro.com commentedHere's updated patch file.
Comment #24
KarlKedrovsky CreditAttribution: KarlKedrovsky at VMLY&R commentedTested by doing the following:
Some tests from the Testing module failed but it looks like the batch process that runs them worked fine.
Comment #27
volegerReroll for 8.6.x.
Reverted changes in
db_next_id
tests.So this patch contains only 1 replacement in
core/includes/form.inc
.Comment #29
Mile23I think this should be
\Drupal::database()
, but there is some contention, so I'm postponing this on #2991337: Document the recommended ways to obtain the database connection objectComment #30
Mile23Comment #31
mondrakeI think this can be unpostponed now.
Comment #33
mondrakeNeed to install the
sequences
schema in the deprecation test to testdb_next_id
.Comment #35
mondrake$modules visibility.
Comment #37
mondrakeFix for deprecation test.
Comment #39
mondrakeComment #40
volegerComment #41
volegerComment #42
mondrakePlease do not change these from assertEqual to assertEquals - not in the scope of this issue. In any case, assertEquals requires the expected value as first argument and the actual as second, so in the case of
$this->assertEquals($result, 1001, ...)
they would need to be swapped.EDIT: also, it would probably be better to use assertSame here to check the type of the returned variable too
Comment #43
volegerComment #44
andypostWhy not replace its usage in
core/scripts/generate-d7-content.sh
?Comment #45
mondrake@andypost
core/scripts/generate-d7-content.sh
generates d7 code so that should stay as is. See #2232391: Revert drupal 8 changes made to generate-d7-content.sh and generate-d6-content.sh.Comment #46
andypost@mondrake thanx for pointer, other then test change it looks good to go
Not sure the change makes sense in context #2991337: Document the recommended ways to obtain the database connection object
Comment #47
mondrake#46 changing that test class to inherit from DatabaseTestBase (like all the other test classes in the namespace, except SchemaTest) allows us to leverage the
$connection
property defined in setUp. I think that make sense.If your question is 'how' the $connection property is initialised - fine that will be decided in #2991337: Document the recommended ways to obtain the database connection object, then we can change the setUp method in test base class accordingly and all the extending classes will benefit.
Comment #48
andypostThen it looks good to go!
Comment #50
mondrakeComment #51
mondrakeRerolled.
Comment #52
catchCommitted 6b61e10 and pushed to 8.7.x. Thanks!