FriendsOfCake / bootstrap-ui

CakePHP: Transparently use Bootstrap
MIT License
340 stars 145 forks source link

FlashHelper doesn't seem to fix up the error class when it's not an array #185

Open moldedjelly opened 7 years ago

moldedjelly commented 7 years ago

This is a:

What you did

I noticed that the Flash messages with general class "error" weren't being fixed up to "alert-danger". So I updated line 72 of src/View/Helper/FlashHelper.php, adding an else for when it's not an array:

            if (is_array($message['params']['class'])) {
                $message['params']['class'][] = 'alert-' . $class;
            } else {
                $message['params']['class'] = 'alert-' . $class;
            }

I really hope this helps.

moldedjelly commented 7 years ago

sorry, had to modify the code to:

        if (is_array($message['params']['class'])) {
            $message['params']['class'][] = 'alert-' . $class;
        } else {
            $message['params']['class'] = 'alert alert-' . $class;
        }
ThoWen77 commented 3 years ago

Hi,

I think, this bug is still there. At least for me.

The modified code example works for me. But I don't like to mess around in others people code, especially if a simple update can wipe out the changes again.

Any chance that this will be fixed?

ndm2 commented 3 years ago

What BootstrapUI version are you using, and how exactly can this be reproduced?

ThoWen77 commented 3 years ago

BootstrapUI: 3.0.1 CakePHP: 4.2.3

To reproduce this:

Option1: Call any page that requires a login without being logged in, one will be redirected to the login page and the flash message "You are not authorized to access that location" is shown as plain text. The login page uses the signin layout from layout/TwitterBootstrap.

Here' the HTML from the message: <div role="alert" class="error">You are not authorized to access that location.</div>

Option2: While being logged in, try to call a "forbidden" page. The same message as above will be shown. The pages use the dashboard layout from layout/TwitterBootstrap.

These messages are generated from the AuthComponent.

Other error flashes (generated by my own code) are shown as expected (light red background, close-X at the right side...).

Example: $this->Flash->error(__('The data could not be saved. Please, try again.'));

All these cases finally lead to a call of FlashComponent::set($message, $options)

Option1 and 2:

$message = "You are not authorized to access that location."
$options = [
  "element" => "error",
  "key" => "flash",
  "params" => [ "class" => "error" ]
]

Own-generated messages:

$message = "The data could not be saved. Please, try again."
$options = [
  "element" => "error"
]

I don't know, if, how and why the different $options causes the problem.

ndm2 commented 3 years ago

So it looks like the auth component sets the class option as string, it's basically the same as your first code example, and currently the helper will only look for bootstrap theme classes in such a case. While the component is deprecated, it would probably still be good if it were supported.

Your second example works fine for me, it's similar to the options that calling error() would create, so I'm not sure what the problem would be here, or did I misunderstand what that example produces for you?

ThoWen77 commented 3 years ago

The first code example (Option 1 and 2) show the call parameters, with that the AuthComponent calls FlashComponent::set($message, $options). That call causes the "text only" error flash messages.

The second code example (Own-generated messages) shows the call parameters of FashComponent::set, when I call $this->Flash->error() in a controller method. That call shows correctly designed error flash messages (red background, round egdes, X for closing the message). So this a counterexample. I tried to find the (relevant) difference between these two error flash situations.

If I mess around with the AuthComponent loading in AppController::initialize, I can get correct error flash messages from there too: (I set the flash config as close as possible to the default, but ommit the class definition)

$this->loadComponent('Auth', [ ... ,
            'flash' => [
                'element' => 'error',
                'key' => 'flash',
                'params' => [ ]
            ]
        ]);

That's the HTML: <div role="alert" class="alert alert-dismissible fade show alert-danger"><button type="button" class="close" data-dismiss="alert" aria-label="Close"><span aria-hidden="true">×</span></button>You are not authorized to access that location.</div>

I still have to check if this has unwanted side effects, but it looks like a workaround to me. :)

If I use the following configuration (settting the class option as array), I get red flash messages, but no rounded edges, no margin/padding and no Close-X:

            'flash' => [
                'element' => 'error',
                'key' => 'flash',
                'params' => ["class" => ["error"] ]
            ]

Here's the resulting HTML: <div role="alert" class="error alert-danger">You are not authorized to access that location.</div>

ndm2 commented 3 years ago

I see. Looking into this further, I don't think there's too much that can be done right now.

Being it as an array or as a string, the class option allows to prevent the default classes configured for the plugin's flash helper to be used (alert, alert-dismissible, fade, show), with the string variant even making it possible to suppress the "element to alert class" behavior, and I would consider that the intended behavior, so personally I wouldn't want to see this change in a minor release.

Adding support for the special case of the error class as used by the auth component might be considered reasonable though.