Problem/Motivation

Steps to reproduce:
Install standard profile
Export config
Reinstall minimal
Hack site uuid (drush cset system.site uuid 'whatever the uuid from the original install was')
Run import

Exceptions will be thrown during route rebuilding, during HttpKernel:terminate()

Issues like #2325185: Convert views_element_info() to Element classes throw further exceptions, which are actually caught by the testbot.

This doesn't happen during module uninstall, since ModuleHandler::uninstall() calls \Drupal::service('plugin.cache_clearer')->clearCachedDefinitions() directly.

Proposed resolution

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#19 dfac-2326409-19-interdiff.txt431 bytesBerdir
#19 dfac-2326409-19.patch11.94 KBBerdir
#17 dfac-2326409-17.patch11.93 KBBerdir
#4 dfac-2326409-3.patch581 bytestim.plunkett
#1 dfac-2326409-1.patch973 bytestim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Active » Needs review
FileSize
dfac-2326409-1.patch973 bytes

Unfortunately this doesn't actually resolve the issue. The code continues to run, and hits a different exception. Let's see if that shows up on tests.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Need to study some more, but in any case, upfront:

Touching the order of operations in dfac() isn't the solution. dfac() is designed for same-state cache flushing + application rebuilding.

The operations in dfac() are performed in exactly the order that is required for its intended use-case. Installation requires a different order. Uninstallation requires the reverse order of Installation (simplified).

Due to that, changing the order of operations in dfac() may seemingly resolve the problem of the calling code, but will cause failures elsewhere. Most likely, the root cause is that uninstall() calls dfac() in the first place.

Status: Needs review » Needs work

The last submitted patch, 1: dfac-2326409-1.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
FileSize
dfac-2326409-3.patch581 bytes

The bug I'm seeing isn't related to ModuleHandler::uninstall(), but ConfigImporter::flush().

Regardless of all that, this at least is a correct change.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

That code comment was not at all related to its code. Good fix. Shall we wait for a more complete fix or proceed with the current patch?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

This is *a* fix, but it doesn't actually fix the bug I had in mind when I opened this issue. It's certainly not critical on its own...
Not sure what to do.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Did anyone considered throwing an event when clearing these caches?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Title: drupal_flush_all_caches() doesn't clear enough caches during config sync » Caches are not cleared before the kernel terminates during config sync

Okay, let's not pin this on dfac.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Priority: Critical » Major
Status: Needs review » Reviewed & tested by the community

OK, lets just go ahead with the patch as is, for now.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Priority: Major » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Plugin system

Well, this issue is already referred to by several others, and blocks ALL of the hook_element_info conversions (except system).
So I spun out #2327965: drupal_flush_all_caches() doesn't clear all plugin caches to get the quick fix in.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +Pre-AMS beta sprint
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

To clarify #10, per @tim.plunkett, #2327965: drupal_flush_all_caches() doesn't clear all plugin caches does not by itself unblock the hook_element_info() conversions.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Afaik #2313135: setting page_cache_without_database in settings.php prevents the container from being dumped could allows us to get rid of some of those problems.

Berdir queued 1: dfac-2326409-1.patch for re-testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Active » Needs review

Patch #1 is green now, setting back to needs review.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

That got moved to #2327965: drupal_flush_all_caches() doesn't clear all plugin caches

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
FileSize
dfac-2326409-17.patch11.93 KB

So @timplunkett said that his patch in here didn't fix the issue for him originally. Removed all the the hook_element_info() and enable @RenderElement, let's see what happens exactly.

Status: Needs review » Needs work

The last submitted patch, 17: dfac-2326409-17.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
dfac-2326409-19.patch11.94 KB
dfac-2326409-19-interdiff.txt431 bytes

That went wrong.

Status: Needs review » Needs work

The last submitted patch, 19: dfac-2326409-19.patch, failed testing.

Berdir queued 19: dfac-2326409-19.patch for re-testing.

almaudoh’s picture

almaudoh CreditAttribution: almaudoh commented

Wow!! Will this also unblock #2028109: Convert hook_stream_wrappers() to tagged services.? I'll try a patch over there...

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review

Green!

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs review » Reviewed & tested by the community

Was it really just #2313135: setting page_cache_without_database in settings.php prevents the container from being dumped?! I'm so glad this passes now.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Title: Caches are not cleared before the kernel terminates during config sync » Annotate render element plugins
Status: Reviewed & tested by the community » Fixed

Retitling to say what the issue is actually doing.

Committed 3b2e57f and pushed to 8.0.x. Thanks!

  • alexpott committed 3b2e57f on 8.0.x 
    Issue #2326409 by Berdir, tim.plunkett: Annotate render element plugins.

Status: Fixed » Closed (fixed)

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