Follow-up to #2754003: Move more cache items from the default to the bootstrap or discovery bin

The theme registry has two different cache entries per theme, the full registry and the runtime registry, which is used on all normal requests and is small.

For a default D8 installation, that's the last cache entry from the default cache bin that's loaded on a normal request with warm cache.

To move this to bootstrap, we need to inject a second cache bin service into the Theme Registry service.

CommentFileSizeAuthor
#4 theme-registry-cache-2761403-4-interdiff.txt670 bytesberdir
#4 theme-registry-cache-2761403-4.patch3.11 KBberdir
#2 theme-registry-cache-2761403-2.patch3.1 KBberdir

Comments

Berdir created an issue. See original summary.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new theme-registry-cache-2761403-2.patch3.1 KB

First patch. Not sure about the optional argument there, made the new cache backend optional for now. Might also turn it around, it is only used for tests anyway.

Status: Needs review » Needs work

The last submitted patch, 2: theme-registry-cache-2761403-2.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new theme-registry-cache-2761403-4.patch3.11 KB
new theme-registry-cache-2761403-4-interdiff.txt670 bytes

Oops.

dawehner’s picture

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

Just check on a plain D8 installation. The runtime cache has 18K in size, the non runtime one, something like 60k, which is all nothing compared to the 200k entries I saw on some other site.

fabianx’s picture

fabianx commented

RTBC + 1

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

  • catch committed 4a95e65 on 8.2.x 
    Issue #2761403 by Berdir: Move runtime theme registry into bootstrap...
catch’s picture

catch
he/him
Primary language English
commented
Status: Fixed » Needs review

Reverted due to #2749955: Random fails in UpdatePathTestBase tests.

Back to CNR.

  • catch committed d189ff0 on 8.2.x 
    Revert "Issue #2761403 by Berdir: Move runtime theme registry into...
hussainweb’s picture

hussainweb
Primary language English
commented

I am not sure if this was the cause for the failures in UpdatePathTestBase tests. I have the commit that reverts this change and still the test fails for me locally. Verbose message for the failures is always this:

In order to run update.php you need to either be logged in as admin or have set $update_free_access in your settings.php.

hussainweb’s picture

hussainweb
Primary language English
commented

Please disregard #11. While debugging for this error, it seemed to fix itself. I am now able to run it multiple times without failing. I don't know if that's conclusive considering it is a random failure.

dawehner’s picture

dawehner
Primary language German
commented

I would have tried to inject a null backend into the theme registry inside \Drupal\Core\Update\UpdateServiceProvider ...

  • catch committed 4a95e65 on 8.3.x 
    Issue #2761403 by Berdir: Move runtime theme registry into bootstrap...
  • catch committed d189ff0 on 8.3.x
    Revert "Issue #2761403 by Berdir: Move runtime theme registry into...

  • catch committed 4a95e65 on 8.3.x 
    Issue #2761403 by Berdir: Move runtime theme registry into bootstrap...
  • catch committed d189ff0 on 8.3.x
    Revert "Issue #2761403 by Berdir: Move runtime theme registry into...
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Assigned: Unassigned » alexpott

@alexpott: I think I remember you saying that this was not actually responsible for those random fails?

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

Any updates on this?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Setting back to RTBC to get feedback from @alexpott, afaik there is nothing else to do here.

  • catch committed 94ae179 on 8.3.x 
    Revert "Revert "Issue #2761403 by Berdir: Move runtime theme registry...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

I just scanned back through that issue, and pretty sure this issue was a red herring with the random fails.

I've reverted the 8.3.x revert, moving this to fixed again!

Status: Fixed » Closed (fixed)

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