Problem/Motivation
While working on #2746541: Migrate D6 and D7 node revision translations to D8 it was discovered that Sql::getHighestId() fails if any of the destination ids are not of type 'integer'.
The master node migration, to be introduced in #2746541: Migrate D6 and D7 node revision translations to D8 3 destination ids; nid, vid and langcode. Currently, getHighestId() throws a LogicException because langcode is a string. Irregardless of the master node migration getHighestId() should not fail if a destination id is not an integer, having a langcode as an identifier seems quite reasonable.
Proposed resolution
Just allow Sql::getHighestId() to work if at least one id is of type integer, not all of them need to be integers.
Remaining tasks
Patch
Add a test
Review
Comment | File | Size | Author |
---|---|---|---|
#45 | 3086238-45.patch | 4.95 KB | quietone |
#45 | interdiff-41-45.txt | 1.79 KB | quietone |
#41 | 3086238-41.patch | 4.8 KB | quietone |
#41 | interdiff-38-41.txt | 3.4 KB | quietone |
#38 | 3086238-38.patch | 6.27 KB | quietone |
Comments
Comment #2
quietone CreditAttribution: quietone as a volunteer commentedComment #4
quietone CreditAttribution: quietone as a volunteer commentedOpted to add a way to select which destination id to get the highest value of. This allows one to pass in the name of the id, as in 'nid' or 'vid', and then that is used in the query. I've also changed the query to use MAX, not sure why it currently doesn't.
Comment #6
quietone CreditAttribution: quietone as a volunteer commentedRework the query.
Comment #7
quietone CreditAttribution: quietone as a volunteer commentedComment #8
quietone CreditAttribution: quietone as a volunteer commentedAnd this is a blocker to #2746541: Migrate D6 and D7 node revision translations to D8.
Comment #9
Gábor HojtsyIs this a new feature (too). The first part of the issue summary sounds like it is an existing bug, but then "we should also add this feature while we are here". Not sure what is the relation between the two?
Comment #10
heddnNit: Id should be ID.
Comment #11
heddnThis needs an update on
HighestIdInterface
as well.Comment #12
quietone CreditAttribution: quietone as a volunteer commentedUpdated the IS in the hopes of clarifying the questions in #9
Fixes for #10 amd #11.
One thing that I wonder about is the changes I've made to the query. I've skimmed through the original issue, #2876085: Before upgrading, audit for potential ID conflicts. In comment #154 heddn agrees that this should handle multiple destination IDs and the query was changed in https://www.drupal.org/project/drupal/issues/2876085#comment-12316496 to order by all destination fields. Since it was only returning the highest value for the first field it didn't matter but if we want the second field, like 'vid', then that sorting isn't guaranteed to produce the result we want. That is why I switched to using MAX. Just want to be clear about that change.
Comment #14
quietone CreditAttribution: quietone as a volunteer commentedOops. Was thinking about catching a bus and not what I was typing.
Comment #16
dinarcon CreditAttribution: dinarcon at Agaric commentedHi @quietone, is it intentional that in HighestIdInterface::getHighestId() that docblock includes `@param string $id_name` while the method signature does not include the `id_name` parameter? I noticed that in `Sql::getHighestId()` the optional param is present and defaults to `NULL`.
Also, in the issue summary it says "it was discovered that Sql::getHighestId() fails if any of the destination ids are not of type 'string'." Shouldn't the last part say "if any of the destination ids are not of type 'integer'."
Comment #17
quietone CreditAttribution: quietone as a volunteer commented@dinarcon, thx. I have corrected the IS. Now, off to bed.
Comment #18
quietone CreditAttribution: quietone as a volunteer commented@dinarcon, How about this?
Although I guess this is not BC since it changes the interface.
Comment #20
quietone CreditAttribution: quietone as a volunteer commentedHopefully just missed one.
Comment #21
larowlanI think the BC way to do this is to deprecate the method without the argument and add a new method with the argument.
An example of where we've done that before - https://www.drupal.org/node/3060969
Comment #22
quietone CreditAttribution: quietone as a volunteer commented@larowlan, thanks that helped a lot.
Now, let's see if I got it right. Well, mostly, this still needs a CR and adding that to deprecation messages.
Comment #23
larowlanThanks, we should probably update the IS too
Code changes look good, will leave it to migrate folks to iron out the best name for the new method.
We also need a deprecation test to ensure the error is triggered.
And obviously, needs to use the actual change notice link, nice work @quietone 😎
Comment #24
quietone CreditAttribution: quietone as a volunteer commented@larowlan, Thanks! Pleased I got so much correct.
One thing I am not clear on. Are you saying there is a test for the expectedDeprecation message and one for the trigger error message?
Is this not sufficient?
Comment #25
larowlanOh, I missed that in the patch - all good
Comment #26
quietone CreditAttribution: quietone as a volunteer commentedAdded draft change record and updated the notices in the patch with its node id.
Comment #27
quietone CreditAttribution: quietone as a volunteer commentedUpdates the IS.
And as I wrote that I wonder if there should be a check to force the first destination id to be an integer. That would limit someone that has a string id first, followed by an integer id. Obviously not a use case in core and a very unlikely case in the wild.
Comment #28
quietone CreditAttribution: quietone as a volunteer commentedThis may not be the way to go. The changes don't help make using the AuditInterface usable when the migration has multiple ids that should be checked.
Comment #29
quietone CreditAttribution: quietone as a volunteer commentedI've decided to reduce the scope of this to only what is needed for #2746541: Migrate D6 and D7 node revision translations to D8. At the time it seemed worth doing both here but alas, no. My local testing of it show that adding the ability to select the destination id to get the highest value of isn't really flexible enough. New issue being made for the other part.
The patch here just fixes it so that a destination id can be a string and one can still use getHighestId(). I have not made an interdiff because the patch is rather small.
edit: clarify first paragraph
Comment #30
quietone CreditAttribution: quietone as a volunteer commentedUpdate IS.
Created #3091004: getHighestId() should be able to use any integer destination id for the follow up work.
Comment #32
benjifisherMostly nits, but I think the first item needs to be fixed.
I think we want to leave the function signature unchanged until #3091004: getHighestId() should be able to use any integer destination id.
I like using
array_search()
instead ofarray_filter()
. But I do not like the long lines. The one where we throw an exception is OK, but can we rewrite the other one? Something likeis a lot easier to read.
Two suggestions, feel free to leave it as is. Use
"sourceid$i"
and"destid$i"
instead of explicit concatenation. Sincecount($destination_ids)
is used three times, maybe make a variable for it.Maybe I am confused, but that does not seem like an accurate description of the two scenarios. We have one or two keys, all of type string.
I am not sure how the coding standards for comments apply to lists, but be consistent in ending punctuation. I think I would make each item a complete sentence with ending punctuation.
As in #10, I think it should be "Destination ID".
Comment #33
quietone CreditAttribution: quietone as a volunteer commented@benjifisher, thanks for the review
1. Oops, that is a mistake. Fixed
2, 3, and 4. Fixed
Comment #34
benjifisher@quietone:
Thanks for those changes!
This all looks good, except for one mismatch between the comments and the code. This confused me:
It took me several minutes to figure out why this throws the expected exception: the code is looking for "integer", not "int". This difference is easy to miss, especially for a PHP coder:
(int) "17" === (integer) "17"
.Please make the comment match the code, and emphasize the distinction between "int" and "integer": something like
As in #10, I am using "ID", not "id", in both scenarios. The second scenario has two types, hence the plural.
Comment #35
quietone CreditAttribution: quietone as a volunteer commented@benjifisher, thanks again. You've got a good eye for detail.
Fixes here for #34.
Comment #36
benjifisher@quietone, you are very gracious! Thanks for this fix.
Comment #37
alexpottI'm not entirely sure about this. I think the we need to ensure that the first field returned by
$this->destinationIdFields()
is an integer - the rest are not significant looking at the way the query is done...This seems to have been considered in #27 but if you did have
the change here is not going to produce the expected results.
Also this shows that we should be adding test coverage of having a string ID as the first ID.
Comment #38
quietone CreditAttribution: quietone as a volunteer commentedI've added a change in getHighestId() to make sure the first destination id used is of type integer. This isn't done in destinationIdFields() since it only getHighestId() that requires the first id to be an integer.
Comment #39
heddnI'm not sure that we want to silently disregard non integer destination ids. We can't be sure the destination is even a Drupal entity that uses numeric serial destination ids. What about UUIDs on non standard destinations.
Comment #40
alexpottI think I should have been clearer... we should only support getHighestId when the first ID is an integer.
Comment #41
quietone CreditAttribution: quietone as a volunteer commentedYea, I was uncomfortable with that but I thought it was the intention. This patch ensures the first ID is an integer.
Comment #42
alexpottI had to stare at this test case for a while to work out why we expected it to fail. It's because
int
!=integer
. Do we ever expect the type to beint
or was this a made up value for more explicit testing?Comment #43
quietone CreditAttribution: quietone as a volunteer commentedbenjifisher asked that in #34 and the comment was updated accordingly. Honestly, I probably used 'int' because I have made that mistake before.
Comment #44
alexpottAh I see... is these were array keys of the tests then it make PHPUnit output great and the scenarios really obvious. We could do the same with the other data provider.
I.e.
Comment #45
quietone CreditAttribution: quietone as a volunteer commentedFixes for #44.
Comment #46
heddnI think we be good again.
Comment #47
benjifisherI have not had a chance to review the updates since #37, but this part of #42 makes me feel better:
I had the same problem.
Comment #48
alexpottCommitted and pushed 02f1c210f7 to 9.0.x and a10cb97a8e to 8.9.x. Thanks!
Going to ask other committers about backporting this to 8.8.x
Comment #51
quietone CreditAttribution: quietone as a volunteer commentedLook what I found this morning! This should be 'Ensure that the first ID is an integer'. Sorry, not able to fix it now.
Comment #52
alexpott@quietone indeed. Hotfixed...
Comment #55
quietone CreditAttribution: quietone as a volunteer commented@alexpott, thank you!
Comment #56
catchThis comment isn't a sentence.
Comment #57
alexpott@catch +1'd the backport and as per #52 that non-sentence has been hotfixed.
Comment #59
Wim LeersPublished the change record: https://www.drupal.org/node/3089609
EDIT: Unpublished, because it looks like the committed changes deviate from the change record? It seems that we don't need a change record anymore, since it's now simply a bugfix?
Comment #60
quietone CreditAttribution: quietone as a volunteer commentedYes, the change record is not needed.
Comment #61
Wim LeersCool, deleted it :)