Closed (fixed)
Project:
Drupal core
Version:
8.7.x-dev
Component:
layout_builder.module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
25 Mar 2019 at 10:12 UTC
Updated:
7 Jun 2019 at 06:19 UTC
Jump to comment: Most recent, Most recent file



Comments
Comment #2
sam152 commentedI'm not quite sure what the fix for this is. Perhaps the deriver could only create blocks for entity types that had layout builder enabled, or perhaps there is a way to
__wakeupa context definition without callinginitializeEntityContextDefinition?Comment #3
sam152 commentedI have spun off #3043330: Reduce the number of field blocks created for entities (possibly to zero) to look into reducing the size of the block plugin list.
Comment #4
tim.plunkettComment #5
sam152 commentedComment #6
tedbowNot sure if this will work but we didn't call
\Drupal\Core\Plugin\Context\ContextDefinition::initializeEntityContextDefinition()until we actually need to accessentityContextDefinitioninternally.This is will break
\Drupal\Tests\Core\Plugin\Context\EntityContextDefinitionDeprecationTest::testSerialization()because it assumesentityContextDefinitionwill be set as soon as the object is constructed.Interested to see what else it breaks.
Comment #8
tedbow@Sam152 could you run your profiling with this patch and see if it helps at all?
Comment #9
sam152 commentedThat's a dramatic speed-up, blackfire comparison here. Looks like a 20% speed-up of the whole page when clicking the "Add block" button.
Pulling out the relevant parts from the profiler, before:
After:
Comparison:
Comment #10
sam152 commentedI think a suitable fix for the test case might simply be to add a call to
getConstraints. The test is using reflection, so I don't think it's testing a public API. Asserting it's there after triggering an API call seems appropriate:Comment #11
tedbowYep here is #10 added
Comment #12
tim.plunkettThat is significant, well spotted @Sam152 and elegant fix @tedbow!
Comment #13
sam152 commentedGood stuff, thanks for the review and fix!
Comment #14
tedbowI removed the call to
setConstraints()because it would cause an infinite loop call toinitializeEntityContextDefinition()I thought it didn't matter but realized the object could have non-empty
$constraints.I think the solution is
->setConstraints($this->constraints)to avoid the infinite loop.Comment #15
sam152 commentedDoes the fix in #14 need a test? I had a play and this seems enough to ensure the constraints are added during initialization:
Beyond that, looks ready to me.
Comment #16
tedbow@Sam152 good idea about the test
Comment #17
sam152 commentedGood stuff, back to RTBC.
Comment #18
tim.plunkettDeserves a fleshed out issue summary
Comment #20
sam152 commentedFail was a fluke. Updating issue summary.
Comment #22
tedbow#21 was JS testing fail error in
Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTestRetesting
Comment #25
larowlanCommitted 506d92f and pushed to 8.8.x. Thanks!
c/p as c6171b948c to 8.7.x