Follow up to:

#1606794: Implement new routing system
#1786460: Add the lock API to the DIC

The MatcherDumper class needs to be adjusted to use the locking framework to avoid race conditions on rebuild. That must be done via pure injection so that the MatcherDumper is still unit testable.

CommentFileSizeAuthor
#10 router-builder-lock-1798222-10.patch3.65 KBBerdir
#10 router-builder-lock-1798222-10-interdiff.txt891 bytesBerdir
#6 router-rebuild-lock-1798222-6.patch2.78 KBBerdir
#6 router-rebuild-lock-1798222-6.patch2.78 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

Crell CreditAttribution: Crell commented
Priority: Normal » Critical

Per catch's request.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Component: base system » routing system
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Postponed » Active

No reason to postpone this. Also #1642062: Add TempStore for persistent, limited-term storage of non-cache data adds the lock system to the DIC.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

And it has been commited now, so even less reasons to postpone it :)

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Assigned: Unassigned » Berdir

I actually have an almost finished patch for this.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Active » Needs review
FileSize
router-rebuild-lock-1798222-6.patch2.78 KB
router-rebuild-lock-1798222-6.patch2.78 KB

Should be something like this I guess?

Passes the lock into the service, implementation is taken from menu_rebuild and adjusted.

Edit: Accidental double upload, it's the same file.

Status: Needs review » Needs work

The last submitted patch, router-rebuild-lock-1798222-6.patch, failed testing.

Crell’s picture

Crell CreditAttribution: Crell commented

That looks right to me visually. No idea why testbot disagrees.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Upgrade tests. I guess we have a another service definition of router.builder in update.php which needs to be updated as well.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
router-builder-lock-1798222-10-interdiff.txt891 bytes
router-builder-lock-1798222-10.patch3.65 KB

This should fix the tests.

Crell’s picture

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

So it does.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Patch looks consistent with what we do in menu.inc in D7 today.

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

Updated issue summary.