Problem

  • #2145041: Allow dynamic routes to be defined via a callback changed the provider of existing routes, which results in the following errors/exceptions when the router is rebuilt:
    PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'field_ui.instance_edit_comment' for key 'PRIMARY' in core\lib\Drupal\Core\Database\Statement.php on line 54
{main}( )
drupal_rebuild( )
drupal_flush_all_caches( )
Drupal\Core\Routing\RouteBuilder->rebuild( )
Drupal\Core\Routing\MatcherDumper->dump( )
Drupal\Core\Database\Driver\mysql\Insert->execute( )
Drupal\Core\Database\Connection->query( )
Drupal\Core\Database\Statement->execute( )
execute ( )

    IntegrityConstraintViolationException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'field_ui.instance_edit_comment' for key 'PRIMARY': INSERT INTO {router} (name, provider, fit, path, pattern_outline, number_parts, route) VALUES (...snip...)
{main}( )
drupal_rebuild( )
drupal_flush_all_caches( )
Drupal\Core\Routing\RouteBuilder->rebuild( )
Drupal\Core\Routing\MatcherDumper->dump( )

Proposed solution A

  1. Adjust MatcherDumper->dump() to not only delete existing {router} rows with the same 'provider', but also all rows having the same route 'name' as the new routes to be inserted.

Proposed solution B

  1. Use a MergeQuery instead of InsertQuery in MatcherDumper->dump().

Proposed solution C

  1. Make the rebuild script truncate the entire {router} table before rebuilding the router.
CommentFileSizeAuthor
#19 interdiff-2160811-19.txt2.12 KBdamiankloip
#19 2160811-19.patch6.87 KBdamiankloip
#11 drupal8.rebuild-router.11.patch6.45 KBsun
#7 matcher_dumper-2160811.patch3.36 KBdawehner
#7 interdiff.txt2.57 KBdawehner
#3 drupal8.rebuild-router.3.patch1.37 KBsun
#1 drupal8.rebuild-router.1.patch927 bytessun

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new drupal8.rebuild-router.1.patch927 bytes

Attached patch implements proposed solution A.

Status: Needs review » Needs work

The last submitted patch, 1: drupal8.rebuild-router.1.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new drupal8.rebuild-router.3.patch1.37 KB

Sorry, missed the fact that $this->routes is a RouteCollection.

Crell’s picture

Crell commented
Status: Needs review » Reviewed & tested by the community

Of the available options I agree A is the best on the table. Thanks, sun!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

I imagine the answer is no, but is there any possible way of providing test coverage for this?

Crell’s picture

Crell commented

I think when the amount of code needed to test reaches a certain multiplier of the code being tested, it's no longer worth it. I'm pretty sure this would be past that threshold even even I could think of a way to do so (which off hand I can't).

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new interdiff.txt2.57 KB
new matcher_dumper-2160811.patch3.36 KB

I think when the amount of code needed to test reaches a certain multiplier of the code being tested, it's no longer worth it. I'm pretty sure this would be past that threshold even even I could think of a way to do so (which off hand I can't).

Well, if you would apply this logic everywhere all unit tests should be thrown away immediately.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

btw:

+++ b/core/lib/Drupal/Core/Routing/MatcherDumper.php
@@ -111,9 +113,17 @@ public function dump(array $options = array()) {
+      if ($names) {
+        $or->condition('name', $names);
+      }
...
+      $delete->execute();
+
       $insert->execute();

I wasn't sure whether the dumper gets also invoked in case the RouteCollection is empty.

Thus, I used the defensive approach of checking whether $names is non-empty before adding the condition.

[OT: When I wrote that condition code, I wondered whether we couldn't make Condition smart enough to figure out itself whether the argument array of an IN query condition is empty → if so, do not add a malformed SQL WHERE clause. Just a thought.]

But more importantly:

If the RouteCollection can, in fact, be empty, then I can only assume that the subsequent INSERT query will fail and blow up, too?

That edge-case could happen if a module has removed all routes, after providing some routes previously, no?

In that case, we'd still want to execute the DELETE query, so as to remove the stale/obsolete routes of the provider, but not the INSERT query.

In essence, we'd wrap the INSERT query execution into the same new condition, like so?

if ($names) {
  $insert->execute();
}

Thoughts?

Crell’s picture

Crell commented

OK, daniel's test-fu is stronger than mine, apparently.

sun: I'd check for an empty RouteCollection right off, and special case that whole code path. I think that would be simpler over-all, since that's a really really short code path (just firing a delete).

dawehner’s picture

dawehner
Primary language German
commented

For a short moment I thought we should tell the dev that he might want better to return some routes, though I guess there are usecases for modules to provide just an empty collection, so I think it is better to be safe here and just do the changes silently.

@sun
Both how we can fix the Condition object we could also fix the Insert object and not execute something if there are no values to insert.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new drupal8.rebuild-router.11.patch6.45 KB

Added the empty RouteCollection check in the way @Crell suggested.

Sorry for the much larger diff; since $this->routes is ought to be flushed/emptied after dumping, I turned the whole thing into an if/else structure.

Interdiff is pointless. No other changes in that code though. Only adjusted the new test accordingly.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: drupal8.rebuild-router.11.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Also created #2161943: Throw a helpful exception for empty IN conditions in Database\Query\Condition

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review

11: drupal8.rebuild-router.11.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 11: drupal8.rebuild-router.11.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Weird - I'm not able to reproduce the installation failure locally

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Needs review

11: drupal8.rebuild-router.11.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 11: drupal8.rebuild-router.11.patch, failed testing.

damiankloip’s picture

damiankloip commented
Status: Needs work » Needs review
StatusFileSize
new 2160811-19.patch6.87 KB
new interdiff-2160811-19.txt2.12 KB

I think we also need to take care of the case where an empty route collection is set on the Dumper? Like this.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

So yeah empty indeed does not work on objects.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Awesome, thanks! :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 4f2d2ab and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.