Problem/Motivation

Currently there's no memory limit policy constraint based on routes. This could be useful similarly to the path use case.

Proposed resolution

Create a "memory_limit_policy_route" submodule that creates a constraint based on

  • Specific routes listed in a textarea, one per line (same as the path submodule)
  • A checkbox for admin routes - those with _admin_route option set

Remaining tasks

Review and test the patch attached in this thread.

User interface changes

None other than the addition of the new route-based memory limit policy constraint.

API changes

The priority of the KernelEvents::REQUEST memory_limit_policy subscriber was changed so that routes are preloaded and available. This is probably not ideal and a TODO note was left in the modules/memory_limit_policy_route/src/MemoryLimitPolicyRouteServiceProvider.php file that points in a direction we could further debug and test to only change that priority when the route submodule is enabled.

CommentFileSizeAuthor
#6 interdiff_2-6.txt9 KBerickbj
#6 memory_limit_policy-3275301-6-routes-constraint-support.patch9.29 KBerickbj
#2 memory_limit_policy-3275301-2-routes-constraint-support.patch7.45 KBerickbj

Comments

erickbj created an issue. See original summary.

erickbj’s picture

erickbj commented
Status: Active » Needs work
StatusFileSize
new memory_limit_policy-3275301-2-routes-constraint-support.patch7.45 KB
erickbj’s picture

erickbj commented
Issue summary: View changes
vbouchet’s picture

vbouchet
Location Toulouse
commented

Hi Erickbj,

Thanks for the patch.

Here is my review:

  • Should we use isAdminRoute() to determine if the route is an admin's one?
  • I think we should have 2 different constraints (in same module), one for route list, one for admin routes.
  • What is the reasoning behind the change in the EventListener priority? Why a change in both the memory_limit_policy module and in the memory_limit_policy_route submodule?
erickbj’s picture

erickbj commented

Hi vbouchet,

Thanks for the feedback! To your questions:

Should we use isAdminRoute() to determine if the route is an admin's one?

Definetely - I missed that, I'll make that change.

I think we should have 2 different constraints (in same module), one for route list, one for admin routes.

Will make that change.

What is the reasoning behind the change in the EventListener priority? Why a change in both the memory_limit_policy module and in the memory_limit_policy_route submodule?

Changing the EventListener priority was needed because the "current route match" service would always return the route as NULL when the event listener had priority of 100. I guess that's loaded by Drupal core later with prio 0, so when I changed the listener to -1 that worked. I don't know however if this change could cause side effects for other cases or just run too late, that's why I tried exploring the approach of changing the priority from 100 to -1 conditionally via MemoryLimitPolicyRouteServiceProvider, but I couldn't make that work. Solution should be either one of (a) changing the priority directly in MemoryLimitPolicySubscriber or (b) changing it conditionally via MemoryLimitPolicyRouteServiceProvider

erickbj’s picture

erickbj commented
StatusFileSize
new memory_limit_policy-3275301-6-routes-constraint-support.patch9.29 KB
new interdiff_2-6.txt9 KB

@vbouchet changes are done.

I think the only things left to work on are (a) the change in priority (discuss if really need and if so try using the service provider priority alter approach) and (b) maybe remove the "AdminRoute" configuration form.

As I created the "AdminRoute" constraint plugin I realized its configuration form was useless (there's no configuration for that) and I just added a disclaimer instead. I didn't know how to remove the form itself.

vbouchet’s picture

vbouchet
Location Toulouse
commented

@erickbj

Thanks for the change and details.

Regarding the priority, I will give a look but as per the comment:

// High priority for this subscriber to execute it soon enough.
$events[KernelEvents::REQUEST][] = ['onRequest', 100];

It seems the earlier it is executed, the better it is so moving to -1 is probably safe.

Regarding the adminRoute plugin, I will review the wording but it can be use to apply on all the admin routes or on all the non-admin routes.

I will update the issue once I applied your patch and made the adaptations discussed here.

  • vbouchet committed 10ac4f1 on 8.x-1.x authored by erickbj 
    Issue #3275301 by erickbj: Support for route based memory limit policy...
  • vbouchet committed b2bcdd6 on 8.x-1.x
    Issue #3275301: Update the adminRoute constraint to allow negate the...
vbouchet’s picture

vbouchet
Location Toulouse
commented
Status: Needs work » Fixed

I updated the dev branch.

The adminRoute settings form now have a "Negate" check box to apply the constraint on all non-admin routes. I realised it may be a good addition to have a global "Negate" checkbox to negate any constraint instead of having it at contraint level. I will create a separate issue for it.

Thanks again for this great addition.

Status: Fixed » Closed (fixed)

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