When trying to edit an effect with an invalid (inexistent or deleted) effect ID in URL, Drupal returns 500. It should return 401.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

claudiu.cristea’s picture

Status: Active » Needs review
FileSize
1.98 KB

Here's a patch showing this bug.

Status: Needs review » Needs work

The last submitted patch, 2063403-1.patch, failed testing.

claudiu.cristea’s picture

FileSize
5.76 KB

Here's the fix.

claudiu.cristea’s picture

Status: Needs work » Needs review
tim.plunkett’s picture

Title: Crash when edit inexistent effect » Fatal when editing a nonexistent image effect
@@ -65,7 +67,13 @@ public function has($instance_id) {
-      $this->initializePlugin($instance_id);
+      try {
+        $this->initializePlugin($instance_id);
+      }

This should not catch the exception, its up to an individual plugin bag.

@@ -65,7 +67,13 @@ public function has($instance_id) {
+      catch (PluginException $e) {
+        $null = NULL;
+        return $null;

This looks wrong

@@ -344,6 +346,11 @@ function testEditEffect() {
+    // Try to edit an inexisting effect.

nonexistent

Status: Needs review » Needs work

The last submitted patch, no-effect-2063403-3.patch, failed testing.

andypost’s picture

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
6.7 KB
7.35 KB

Just finished patch when you posted :)

claudiu.cristea’s picture

This needs reviews or RTBC

dawehner’s picture

  1. +++ b/core/lib/Drupal/Component/Plugin/Exception/UnknownPluginException.php
    @@ -0,0 +1,31 @@
    + * Definition of Drupal\Component\Plugin\Exception\UnknownPluginException.
    

    Should be @file\nContains ...

  2. +++ b/core/lib/Drupal/Component/Plugin/Exception/UnknownPluginException.php
    @@ -0,0 +1,31 @@
    +  public function __construct($instance_id, $message = '', $code = 0, \Exception $previous = NULL) {
    

    It is a bit odd that the message is not the first parameter of the exception.

  3. +++ b/core/modules/image/lib/Drupal/image/Form/ImageEffectFormBase.php
    @@ -57,7 +58,12 @@ public function getFormID() {
    +    catch (UnknownPluginException $e) {
    +      throw new NotFoundHttpException();
    +    }
    

    It would be great to display that the image effect could not be created/does not exist.

claudiu.cristea’s picture

@dawehner,

It is a bit odd that the message is not the first parameter of the exception.

I know but I need a way to pass the plugin id without a message by default. It's not against any rule.

Fixed others, see interdiff.

claudiu.cristea’s picture

Assigned: Unassigned » claudiu.cristea

Yay, passed.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

As #10 is addressed and #1 contains test only patch. So I think it is ready for RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Needs work since #1969572: Make Uuid a service has landed

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
810 bytes
5.97 KB

Here with uuid as service.

Status: Needs review » Needs work

The last submitted patch, no-effect-2063403-15.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
7.21 KB

Oh, missed Drupal\Component\Plugin\Exception\PluginException.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @claudiu.cristea for fixing this. Green with Uuid service so back to RTBC.

Xano’s picture

#17: no-effect-2063403-17.patch queued for re-testing.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed a6d19bf and pushed to 8.x. Thanks!

Removed unnecessary use statements during commit.

diff --git a/core/modules/image/lib/Drupal/image/Form/ImageEffectFormBase.php b/core/modules/image/lib/Drupal/image/Form/ImageEffectFormBase.php
index fc0f547..0df59dd 100644
--- a/core/modules/image/lib/Drupal/image/Form/ImageEffectFormBase.php
+++ b/core/modules/image/lib/Drupal/image/Form/ImageEffectFormBase.php
@@ -12,8 +12,6 @@
 use Drupal\image\ImageStyleInterface;
 use Drupal\Component\Plugin\Exception\UnknownPluginException;
 use Drupal\Component\Utility\String;
-use Symfony\Component\DependencyInjection\ContainerInterface;
-use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;

 /**

Status: Fixed » Closed (fixed)

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