Problem/Motivation

Function Name Calls Calls% Incl. Wall Time
(microsec)		 IWall% Incl.
MemUse
(bytes)		 IMemUse% Incl.
PeakMemUse
(bytes)		 IPeakMemUse%
Current Function
Drupal\views\Routing\ViewPageController::create 1 8.3% 434 0.5% 119,512 0.7% 46,816 0.3%
Exclusive Metrics for Current Function 8 1.8% 1,760 1.5% 192 0.4%
Parent function
Drupal\Core\DependencyInjection\ClassResolver::getInstanceFromDefinition 1 100.0% 434 100.0% 119,512 100.0% 46,816 100.0%
Child functions
Symfony\Component\DependencyInjection\Container::get 2 50.0% 306 70.5% 40,480 33.9% 45,288 96.7%
Drupal\Core\Entity\EntityManager::getStorage 1 25.0% 119 27.4% 76,576 64.1% 1,336 2.9%
Drupal\views\Routing\ViewPageController::__construct 1 25.0% 1 0.2% 696 0.6% 0 0.0%

ViewPageController gets the entity storage of the view without doing anything with it

Proposed resolution

Get rid of $storage, $executableFactory, __construct, create as well as adapt parts of the ViewPageControllerTest

Remaining tasks

User interface changes

API changes

Data model changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it improves performance a bit
Issue priority Normal because the impact is not that high.
Disruption No disruption
CommentFileSizeAuthor
#8 interdiff-3-8.txt2.01 KBmartin107
#8 viewPage-2541084-8.patch4.96 KBmartin107
#3 viewPage-2541084-1.patch4.49 KBmartin107
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

martin107’s picture

martin107 CreditAttribution: martin107 commented
Assigned: Unassigned » martin107

I am working on this now.

joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
CreditAttribution: joshi.rohit100 commented

So fast :)

martin107’s picture

martin107 CreditAttribution: martin107 commented
Status: Active » Needs review
FileSize
viewPage-2541084-1.patch4.49 KB

Stripped out code,

ViewPageControllerTest passes locally after the changes.

To be honest I would be surprised if there were no other failing tests, so I will leave this assigned to me and will jump on any failures.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Tag1 Consulting for Drupal Association commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Quickfix

Nice work!

martin107’s picture

martin107 CreditAttribution: martin107 commented
Assigned: martin107 » Unassigned

Oh well the only jumping I will be doing today is on a suitcase...

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Nice clean-up :)

But in doing so, this patch does not clean up blank lines that now make sense to remove:

+++ b/core/modules/views/tests/src/Unit/Routing/ViewPageControllerTest.php
@@ -56,22 +42,14 @@ class ViewPageControllerTest extends UnitTestCase {
+

+++ b/core/modules/views/tests/src/Unit/Routing/ViewPageControllerTest.php
@@ -56,22 +42,14 @@ class ViewPageControllerTest extends UnitTestCase {
 

@@ -102,8 +80,6 @@ public function testPageController() {
 

@@ -139,8 +115,6 @@ public function testHandleWithArgumentsWithoutOverridden() {
 

@@ -179,8 +153,6 @@ public function testHandleWithArgumentsOnOverriddenRoute() {
martin107’s picture

martin107 CreditAttribution: martin107 commented
Assigned: Unassigned » martin107

pounce

martin107’s picture

martin107 CreditAttribution: martin107 commented
Assigned: martin107 » Unassigned
Status: Reviewed & tested by the community » Needs review
FileSize
viewPage-2541084-8.patch4.96 KB
interdiff-3-8.txt2.01 KB

1) #6 fixed.

2) I have shamelessly added a out of scope fixup. That is I have removed a redundant use statement from ViewPageControllerTest.php
I hope that is ok - it is a ultra trivial fixup to review. The test still passes locally.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community

That seems fine.

alexpott’s picture

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

Let's do less. Committed 0ba8bfe and pushed to 8.0.x. Thanks!

  • alexpott committed 0ba8bfe on 8.0.x 
    Issue #2541084 by martin107, dawehner: ViewPageController does not need...

Status: Fixed » Closed (fixed)

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