Problem/Motivation

When exporting context configuration with Blocks reactions, not all dependencies are added.

Proposed resolution

Process the Blocks collection and add instances dependencies.

Remaining tasks

  • Work on a solution
  • Review
  • Add tests? (The project doesn't seem to have test coverage)
  • Commit

User interface changes

Nope.

API changes

Nope.

Data model changes

Nope, but on exporting the generated config will have the dependencies property filled in.

CommentFileSizeAuthor
#21 2926843-21.patch3.2 KBpaulocs
#21 interdiff-20-21.txt663 bytespaulocs
#20 2926843-19.patch3.2 KBpaulocs
#19 2926843-19.patch1.09 KBmanish-31
#9 2926843-9.patch1.2 KBgambry
#2 missing-dependencies-to-content-2926843-2.patch1.08 KBJoao Sausen
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Joao Sausen created an issue. See original summary.

Joao Sausen’s picture

Joao Sausen CreditAttribution: Joao Sausen at Dexa commented
FileSize
missing-dependencies-to-content-2926843-2.patch1.08 KB

For now the only content I can think of that can be added is block_content.

maaty388’s picture

maaty388 CreditAttribution: maaty388 commented

Replace the line 388 with this

$entity = reset($this->entityTypeManager()->getStorage($entity_type)->loadByProperties(['uuid' => $uuid]));

Just one depency injection...
I tested this.
Also where can I see if depencies are getting exported?
I went debugging and saw that something is added to dependency array.

Joao Sausen’s picture

Joao Sausen CreditAttribution: Joao Sausen at Dexa commented

Just export and the the yml file generated, you will be able to see if there is a content entry on dependencies, it will be something like:

dependencies:
  content:
    - 'block_content:basic:7bfea94e-f845-43a1-b598-5dc3dad2cbdd'
maaty388’s picture

maaty388 CreditAttribution: maaty388 commented

I tested this patch and its working.
I added block Page title and it was not exported to the config because this is working just for block_content blocks.
Should depencies for blocks get exported as well?

Joao Sausen’s picture

Joao Sausen CreditAttribution: Joao Sausen at Dexa commented

It is maybe a better idea to add a dependency to the block directly, if its a custom block, it will have a dependency in the block content already. What do you think? I will work on it when I have time.

maaty388’s picture

maaty388 CreditAttribution: maaty388 commented

Yes I think that will be much better :)

gambry’s picture

gambry
Primary language Italian
Location Milan
CreditAttribution: gambry at manifesto commented
Status: Active » Needs work

Nice work guys.

But I think the dependency calculation should live inside each Condition/Reaction plugin. The context calculateDependencies() should only collect the condition/reaction plugins instances dependencies and merge them to the parent::calculateDependecies(), if any ConfigEntityBase::calculateDependencies() already process each condition/reaction instance calculateDependencies(), it just a matter of adding some logic, as at the moment it just return an empty array .

gambry’s picture

gambry
Primary language Italian
Location Milan
CreditAttribution: gambry at manifesto commented
Version: 8.x-4.0-beta2 » 8.x-1.x-dev
Status: Needs work » Needs review
Issue tags: +Needs issue summary update
FileSize
2926843-9.patch1.2 KB

Attached patch add dependencies for block using a PluginDependencyTrait(). This dependencies are only for the Blocks ContextReaction plugin.
It adds all required block dependencies: configs, content and modules.

This issue summary needs to be update, and maybe follow up must be create to add dependency for example on the selected theme for the Theme ContextReaction plugin.

Also settings 8.x-1.x as target branch due #2933080: 8.x-4.x branch is attached to 7.x code.

gambry’s picture

gambry
Primary language Italian
Location Milan
CreditAttribution: gambry at manifesto commented
Issue summary: View changes
Issue tags: -Needs issue summary update

Updating the issue summary.

gambry’s picture

gambry
Primary language Italian
Location Milan
CreditAttribution: gambry at manifesto commented
Title: Missing dependencies to content » Missing dependencies for ContextReaction\Blocks

Updating the issue title to reflect what the work on this issue refers to (ContextReaction\Blocks dependencies).

BarisW’s picture

BarisW CreditAttribution: BarisW at LimoenGroen commented
Status: Needs review » Reviewed & tested by the community

Can we have this patch in please? We need it on every project we work on, to make sure blocks created on a local environment will get created on other environments using config management

BarisW’s picture

BarisW CreditAttribution: BarisW at LimoenGroen commented

Please commit, so we don't have to use patches for our deployments anymore.

  • boshtian committed 10ffa5d on 8.x-4.x authored by gambry 
    Issue #2926843 by Joao Sausen, gambry, maaty388: Missing dependencies...
boshtian’s picture

boshtian CreditAttribution: boshtian at Agiledrop - Your Trusted Drupal Teammates commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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

lamp5’s picture

lamp5
Primary language Polish
Location Rzeszow
CreditAttribution: lamp5 at abventor commented

Hi quys. I think that something went wrong and this issue still exists. I can see that calculateDependency method is never called so we have no dependencies for our blocks. I tested patch from #2 and it works but need reroll.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
CreditAttribution: paulocs at CI&T commented
Status: Closed (fixed) » Needs work

I'll provide a patch for it.

manish-31’s picture

manish-31 CreditAttribution: manish-31 for DrupalFit commented
Status: Needs work » Needs review
FileSize
2926843-19.patch1.09 KB

I have created a reroll patch for 8.x-4.x, needs review.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
CreditAttribution: paulocs at CI&T commented
FileSize
2926843-19.patch3.2 KB

I created a new patch to add the dependency.
The problem of patch #2 is that it calculates the block dependency in the context entity and it is not right IMHO.
In the patch that I attached, it adds dependency for conditions and reactions.

Please review it.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
CreditAttribution: paulocs at CI&T commented
FileSize
interdiff-20-21.txt663 bytes
2926843-21.patch3.2 KB

Small documentation fix.

guilhermevp’s picture

guilhermevp CreditAttribution: guilhermevp at CI&T commented
Version: 8.x-1.x-dev » 8.x-4.x-dev
Status: Needs review » Reviewed & tested by the community

Patch #21 adds dependency successfully. Moving to RTBC and updating branch.

  • paulocs committed 66dcf94 on 8.x-4.x 
    Issue #2926843 by paulocs, Joao Sausen, gambry, manish-31, maaty388,...
paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
CreditAttribution: paulocs at CI&T commented
Status: Reviewed & tested by the community » Fixed

Fixed!
Thanks.

Status: Fixed » Closed (fixed)

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