Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
1 task done
moldedjelly opened this issue Feb 16, 2017 · 7 comments
Open
1 task done
Labels

Comments

@moldedjelly
Copy link

This is a:

  • bug

  • BootstrapUI Version: v0.5.0

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
Copy link
Author

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
Copy link

ThoWen77 commented Feb 3, 2021

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
Copy link
Collaborator

ndm2 commented Feb 3, 2021

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

@ThoWen77
Copy link

ThoWen77 commented Feb 3, 2021

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
Copy link
Collaborator

ndm2 commented Feb 3, 2021

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
Copy link

ThoWen77 commented Feb 4, 2021

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
Copy link
Collaborator

ndm2 commented Feb 4, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants