Problem/Motivation

#2354889: Make block context faster by removing onBlock event and replace it with loading from a ContextManager changes context providers to be lazy loaded.
This changed the behaviour of ConfigImportAllTest in so far as midway through the install process, comment_entity_storage_load() is fired whilst the container is being rebuilt. At this stage the comment module is in the process of being enabled, and the current user is being determined in order to add the value back onto the rebuilt container. At this point comment module hooks fire but the comment.manager service doesn't exist.

To get around it a check for the service was added.

Proposed resolution

Investigate why the module hooks fire before the service is enabled.

Remaining tasks

Fix

User interface changes

None

API changes

Not known

Data model changes

CommentFileSizeAuthor
#6 2527866-6.patch775 bytesandypost
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost at Skilld commented
Related issues: +#2495087: comment_entity_storage_load() is too expensive on cold caches

Looks we gonna get rid of that function

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Status: Postponed » Active
Issue tags: +Needs beta evaluation

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Version: 8.3.x-dev » 8.4.x-dev
Status: Active » Needs review
Issue tags: -Needs beta evaluation
FileSize
2527866-6.patch775 bytes

Let's see current state

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Needs review » Reviewed & tested by the community

Mystery solved itself?

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Looks like yes) btw queued for 7 & 7.1

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Needs review

I don't see any answer on this issue for the title? The patch just seems to be removing the @todo and the conditional, but does that actually prove that the bug in the summary no longer exists?

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

Yah, ConfigImportAllTest passes without it now, so I think we're good

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs review » Reviewed & tested by the community

I guess this filed too early days and now we have isSyncing() and that fixed things http://cgit.drupalcode.org/drupal/tree/core/modules/comment/comment.modu...

  • xjm committed a63a13b on 8.4.x 
    Issue #2527866 by andypost, larowlan: Investigate why comment.manager...

  • xjm committed 1a6b689 on 8.3.x 
    Issue #2527866 by andypost, larowlan: Investigate why comment.manager...

  • xjm committed dd8733f on 8.3.x 
    Revert "Issue #2527866 by andypost, larowlan: Investigate why comment....
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed

Thanks, #11 makes sense.

Note that the PHP 7 dev version failures are due to #2860663: UserTimeZoneTest fails on PHP 7.0.x-dev and 7.1.x-dev. The 7.1 failure appears to be the segfault (see #2859704: Intermittent segfaults on DrupalCI (some "did not complete due to a fatal error" with no additional info)). But the PHP 7.0 environment passed, so I think we are good there.

I accidentally cherry-picked this to 8.3.x thinking it was just the removal of the @todo, but of course it's also removing the unnecessary method call. So I reverted the 8.3.x commit; leaving this fixed against 8.4.x.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Is there a chance to get this in 8.3.x later?

Status: Fixed » Closed (fixed)

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