Problem/Motivation

In \Drupal\block_place\Plugin\DisplayVariant\PlaceBlockPageVariant::build

$destination = $this->requestStack->getCurrentRequest()->query->get('destination');

Is called in each iteration of the foreach, even though it will be the same every time.

Proposed resolution

Call the method only once outside the foreach.

Possibly it should use the redirect destination service instead of directly checking the query string.

Remaining tasks

doit

User interface changes

none

API changes

none

Data model changes

none

Will need a re-roll when #2787641: Add non-UI mechanism for setting block weight through block forms is committed, however.

CommentFileSizeAuthor
#2 2791335-2.patch3.41 KBpwolanin

Comments

pwolanin created an issue. See original summary.

pwolanin’s picture

pwolanin commented
Status: Active » Needs review
StatusFileSize
new 2791335-2.patch3.41 KB

Changing to use the service like \Drupal\block\Controller\BlockLibraryController does

pwolanin’s picture

pwolanin commented
Title: Optimize repeated method call in PlaceBlockPageVariant::build, us » Optimize repeated method call in PlaceBlockPageVariant::build, use service instead of direct query string
yesct’s picture

yesct commented 
+++ b/core/modules/block_place/src/Plugin/DisplayVariant/PlaceBlockPageVariant.php
@@ -52,14 +53,14 @@ class PlaceBlockPageVariant extends BlockPageVariant {
-  public function __construct(array $configuration, $plugin_id, $plugin_definition, BlockRepositoryInterface $block_repository, EntityViewBuilderInterface $block_view_builder, array $block_list_cache_tags, ThemeManagerInterface $theme_manager, RequestStack $request_stack) {
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, BlockRepositoryInterface $block_repository, EntityViewBuilderInterface $block_view_builder, array $block_list_cache_tags, ThemeManagerInterface $theme_manager, RedirectDestinationInterface $redirect_destination) {

mm. api change.

dawehner’s picture

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

mm. api change.

Given that the block_place module is experimental this is totally fine. IMHO this in general would be also fine in a minor version world, as the constructor is not the public interface.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 2acb2aa to 8.3.x and c88ad1f to 8.2.x. Thanks!

I chose to commit this to 8.2.x as well because place_block is an experimental module added in 8.2.x and improving the constructors before the release of 8.2.0 makes sense.

diff --git a/core/modules/block_place/src/Plugin/DisplayVariant/PlaceBlockPageVariant.php b/core/modules/block_place/src/Plugin/DisplayVariant/PlaceBlockPageVariant.php
index 766796f..b2d3c33 100644
--- a/core/modules/block_place/src/Plugin/DisplayVariant/PlaceBlockPageVariant.php
+++ b/core/modules/block_place/src/Plugin/DisplayVariant/PlaceBlockPageVariant.php
@@ -10,7 +10,6 @@
 use Drupal\Core\Theme\ThemeManagerInterface;
 use Drupal\Core\Link;
 use Symfony\Component\DependencyInjection\ContainerInterface;
-use Symfony\Component\HttpFoundation\RequestStack;
 
 /**
  * Allows blocks to be placed directly within a region.

Fixed on commit.

  • alexpott committed 2acb2aa on 8.3.x 
    Issue #2791335 by pwolanin: Optimize repeated method call in...

  • alexpott committed c88ad1f on 8.2.x 
    Issue #2791335 by pwolanin: Optimize repeated method call in...
yesct’s picture

yesct commented
Issue tags: +Performance
yesct’s picture

yesct commented

Made #2793053: Optimize repeated method call in BlockLibraryController and posted initial patch there.

Status: Fixed » Closed (fixed)

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