Problem/Motivation

Other events triggered by the ConfigImporter has the ConfigImporter object attached to allow listeners to react to the changes it is making. For example, \Drupal\Core\Config\ConfigEvents::IMPORT has it which allows \Drupal\default_content\Config\DefaultContentConfigSubscriber to install default content from modules installed during the import. Because the MissingContentEvent does not have it the Default content module cannot determine what content it is going to create and therefore it can't respond to the event correctly.

See #2849128: Add handling of ConfigEvent::IMPORT_MISSING_CONTENT for the contrib issue this blocks

Proposed resolution

Add the ConfigIMporter object to the event.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3414993

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
Related issues: +#2849128: Add handling of ConfigEvent::IMPORT_MISSING_CONTENT

alexpott opened merge request !6202

andypost’s picture

andypost
he/him
Primary language Russian
commented
Category: Task » Bug report
Status: Active » Needs review

I think we can thread it as a bug because the event is not really a migrate event

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@andypost what does Migrate have to do with it?

andypost’s picture

andypost
he/him
Primary language Russian
commented

Sorry, I mean config import event

-class MissingContentEvent extends Event {
+class MissingContentEvent extends ConfigImporterEvent {
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

But it is a Config import event - is only triggered by the ConfigImporter - see the constructor...

  /**
   * Constructs a configuration import missing content event object.
   *
   * @param array $missing_content
   *   Missing content information.
   */
  public function __construct(array $missing_content) {
    $this->missingContent = $missing_content;
  }
smustgrave’s picture

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

Ran locally and got

Testing /var/www/html/web/core/tests/Drupal/KernelTests/Core/Config

Error : Call to undefined method Drupal\Core\Config\Importer\MissingContentEvent::getConfigImporter()

To show the test-coverage. Couldn't run the test-only feature here.

Test of code change appears fine though and didn't appear to break anything.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: 11.x-dev » 10.2.x-dev
Status: Reviewed & tested by the community » Fixed

Was on the fence about backporting this as it adds a tiny bit of API but this seems unlikely to be unused outside of the ConfigImporter and if backporting allows us to unblock contrib then let's do that.

Committed and pushed eafa856443 to 11.x and 476e65a1e1 to 10.2.x. Thanks!

  • longwave committed 476e65a1 on 10.2.x 
    Issue #3414993 by alexpott: Add ConfigImporter to \Drupal\Core\Config\...

  • longwave committed eafa8564 on 11.x 
    Issue #3414993 by alexpott: Add ConfigImporter to \Drupal\Core\Config\...

longwave closed merge request !6202

andypost’s picture

andypost
he/him
Primary language Russian
commented

Thank you a lot, it is great help for default content!

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented

Hmm, we're using the MissingContentEvent in our unit tests for ECA and they're now failing. Should we add the config importer argument in our tests and ignore the warning about too many arguments in earlier core versions?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Whoops, I thought I searched contrib and found no uses, maybe an error on my part. Happy to revert and rework it if that solves the issue.

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented

Not sure if it's worth it, don't want to hold back some otherwise unblocking improvements. We're "only" using this is a unit test, and could just add that second argument there. It shouldn't break anything for users, I suspect.

  • longwave committed 4f22397c on 10.2.x 
    Revert "Issue #3414993 by alexpott: Add ConfigImporter to \Drupal\Core\...

  • longwave committed 5b72d300 on 10.3.x 
    Revert "Issue #3414993 by alexpott: Add ConfigImporter to \Drupal\Core\...

  • longwave committed f6ea946e on 11.x 
    Revert "Issue #3414993 by alexpott: Add ConfigImporter to \Drupal\Core\...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Fixed » Needs work

Looking at it again, decided to revert; let's add a default value for that new argument so existing callers don't break.

quietone’s picture

quietone commented
Version: 10.2.x-dev » 11.x-dev

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.