yiisoft/yii2-bootstrap5

Alert close button API is incompatible with previous versions

antichris opened this issue · 14 comments

antichris commented

Namely, the close button tag cannot be configured to have any content (label, previously), but the documentation does not explain how is this superior to the prior behavior.

And, regardless of the tag value used, it always gets attribute type="button", which also is a deviation from the previous behavior. That probably is another issue, you're welcome to open one.

What steps will reproduce the problem?

  1. Create a Composer project requiring PHPUnit and this extension:

    composer.json 
    {
	"require": {
		"phpunit/phpunit": "^9",
		"yiisoft/yii2-bootstrap5": "~2.0.0"
	},
	"repositories": [
		{
			"type": "composer",
			"url": "https://asset-packagist.org"
		}
	]
}

  2. Write a test:

    <?php

declare(strict_types=1);

use PHPUnit\Framework\TestCase;
use yii\bootstrap5\Alert;
use yii\web\Application;
use yii\web\View;

final class AlertCloseButtonTest extends TestCase
{
    /** @before */
    public function setUp(): void
    {
        require_once __DIR__ . '/vendor/yiisoft/yii2/Yii.php';

        $app = $this->createMock(Application::class);
        $app->method('getView')->willReturn($this->createMock(View::class));

        Yii::$app = $app;
    }

    public function testLabel(): void
    {
        $tag = 'foo';
        $label = 'bar';

        $pattern = "<div[^>]+>\\s+<({$tag})[^>]+>{$label}</\\1+>\\s+</div>";
        $string = Alert::widget(['closeButton' => compact('tag', 'label')]);

        $this->assertMatchesRegularExpression("(^{$pattern}$)", $string);
    }
}

    Of course the test should pass if the values of $tag and $label are replaced with any other string, as long the regex does not break.

    (Here's also a test for the button type issue, if you feel inclined to see it for yourself) 
    public function testType(): void
{
    $tag = 'a';

    $pattern = <<<EOD
        <div[^>]+>\s*
        <($tag)[^>]+type="button"[^>]*>.*</\\1+>\s*
        </div>
        EOD;
    $string = Alert::widget(['closeButton' => compact('tag')]);

    $this->assertDoesNotMatchRegularExpression("(^{$pattern}$)", $string);
}

    This test also should pass as long as the $tag is anything but button.

  3. Run the test:

    vendor/bin/phpunit --bootstrap=vendor/autoload.php AlertCloseButtonTest.php

What is the expected result?

The test passes with all versions of Bootstrap extension.

What do you get instead?

The test only passes with Bootstrap 3 and Bootstrap 4 extensions, it fails with this one.

Additional info

As it turns out, these very same tests pass even with the brand spanking new next-generation Bootstrap 5 extension.

(with some relatively minor changes) 
diff --git a/composer.json b/composer.json
index 0407097..ca007eb 100644
--- a/composer.json
+++ b/composer.json
@@ -1,7 +1,8 @@
 {
+    "minimum-stability": "dev",
     "require": {
         "phpunit/phpunit": "^9",
-        "yiisoft/yii2-bootstrap5": "~2.0.0"
+        "yiisoft/yii-bootstrap5": "*"
     },
     "repositories": [
         {
diff --git a/AlertCloseButtonTest.php b/AlertCloseButtonTest.php
index 8500d33..a89c888 100644
--- a/AlertCloseButtonTest.php
+++ b/AlertCloseButtonTest.php
@@ -3,21 +3,15 @@
 declare(strict_types=1);
 
 use PHPUnit\Framework\TestCase;
-use yii\bootstrap5\Alert;
-use yii\web\Application;
-use yii\web\View;
+use Yiisoft\Yii\Bootstrap5\Alert;
+use Yiisoft\Widget\WidgetFactory;
 
 final class AlertCloseButtonTest extends TestCase
 {
     /** @before */
     public function setUp(): void
     {
-        require_once __DIR__ . '/vendor/yiisoft/yii2/Yii.php';
-
-        $app = $this->createMock(Application::class);
-        $app->method('getView')->willReturn($this->createMock(View::class));
-
-        Yii::$app = $app;
+        WidgetFactory::initialize();
     }
 
     public function testLabel(): void
@@ -26,7 +20,7 @@ final class AlertCloseButtonTest extends TestCase
         $label = 'bar';
 
         $pattern = "<div[^>]+>\\s+<({$tag})[^>]+>{$label}</\\1+>\\s+</div>";
-        $string = Alert::widget(['closeButton' => compact('tag', 'label')]);
+        $string = Alert::widget()->closeButton(compact('tag', 'label'))->render();
 
         $this->assertMatchesRegularExpression("(^{$pattern}$)", $string);
     }
@@ -40,7 +34,7 @@ final class AlertCloseButtonTest extends TestCase
             <($tag)[^>]+type="button"[^>]*>.*</\\1+>\s*
             </div>
             EOD;
-        $string = Alert::widget(['closeButton' => compact('tag')]);
+        $string = Alert::widget()->closeButton(compact('tag'))->render();
 
         $this->assertDoesNotMatchRegularExpression("(^{$pattern}$)", $string);
     }
simialbi commented

According docs there is no possibility to change close label any more. The ❌ icon will get rendered by background property, so it's going to look strange if you'd have any content in it. The type="button" can get overriden by e.g.

Alert::widget([
    'closeButton' => [
        'tag' => 'a',
        'type' => ''
    ]
]);
antichris commented

According docs there is no possibility to change close label any more.

Sorry, there must be something wrong with my internet or eyes, but I just could not find it. Could you just paste here a direct quotation where it says that?

The type="button" can get overriden by [setting the attribute value to an empty string].

Why do you think this is an example of good API design?

simialbi commented

It's nowhere directly mentioned, but try it yourself. First their example:

<div class="alert alert-warning alert-dismissible fade show" role="alert">
  <strong>Holy guacamole!</strong> You should check in on some of those fields below.
  <button type="button" class="btn-close" data-bs-dismiss="alert" aria-label="Close"></button>
</div>

There is no content inside button tag. Have a look what it looks like if you insert some content:
image
I inserted a red &times; here, now you have &times; twice. One by background, one by content.

Why do you think this is an example of good API design?

It's not, you're right. It's something to be reconsidered to work over.

antichris commented

Sure:

<div class="alert alert-warning alert-dismissible fade show" role="alert">
  <strong>Holy guacamole!</strong> You should check in on some of those fields below.
  <button type="button" class="btn btn-outline-warning" data-bs-dismiss="alert"
    style="position:absolute;top:.5rem;right:.5rem">Dismiss</button>
</div>

And this is what I got:

image

The damn thing works! So, care to elaborate, why should this be made impossible by the design of the API?

simialbi commented

The damn thing works! So, care to elaborate, why should this be made impossible by the design of the API?

Of course it works. With enough customizing everything works. There is no example in any component in the docs to customize the close button, so there is no guarantee it will still work when there is a bootstrap update. So I think it should be discussed: @bizley, @samdark your opinion?

antichris commented

With enough customizing everything works.

Right. But the current design of Alert precludes any such customization — apart from going and plopping the markup for a close button in the body of the alert by hand. It's exactly what this issue is all about.

I'm doubtful there is much added value in discussing this. Simply take what's been done in the next-gen Yii "3" Bootstrap 5 extension, and replicate it here.

rant

Like, I get it — you want to motivate people to abandon the legacy Yii 2 ASAP and migrate to the upcoming Yii architecture, or maybe wean them off using server-side widgets for presentation rendering altogether, force them to draw hard API boundaries between that and the business logic, make them migrate all their front-end components to something like Vue, React or Angular, rendering everything client side or even on dedicated NodeJS front-end servers, using well designed, standardized protocols such as REST or gRPC to communicate between presentation and business microservices, maybe eventually even not use any PHP ever again for anything at all ever...

(a huge intake of breath)

I feel you, bro, I totaly do. They're all noble and commendable goals.

But I don't think making Yii components barely usable unwieldy now is the best way to go about it.

bizley commented

@antichris please tone down you snarky comments and keep the discussion constructive, you are not really helping your cause like that.

I don't necessarily agree that this package must behave the same way as the previous versions. And showing some arbitrary test as a proof that we have similarities in few cases and thus all differencies are bad is just pure logical error.

But...

Yii 2 users are pretty used to the way previous versions of widgets work and we should made it easier for them to move to this version. If some elements of the widgets were used then and it is possible to have them in the v5 version (even though BS5 is not showing them anymore in the examples) we should have them here as well. This is the best moment for such BC adjustments because this package is still fresh.

antichris commented

snarky comments

So, you disagree that presentation concerns should be separated from domain logic, and that PHP is, in fact, a legacy freakshow of language that's wheezing for a final shot in its severely misshapen head? That's okay, I'm perfectly fine with us not sharing the same sense of humor, and your opinion not reflecting mine.

I don't necessarily agree that this package must behave the same way as the previous versions.

As I see it, https://github.com/yiisoft/yii-bootstrap5 has been ignored every time I've brought it up. Are you saying that is also a "previous version" and I've been wrong about it all along?

some arbitrary test as a proof that we have similarities in few cases and thus all differencies are bad

ISTM I have failed to present the point of the test. Here, I'll try to explain: the test verifies a single, very specific feature, behavior that is shared by pretty much all versions of Yii Bootstrap extension, previous and upcoming, this one being the odd one out.

It does not just prove "similarities in [a] few cases". It tests an actual, specific behavior that was implemented by someone (AFAICT, it was here) who wrote those lines on purpose (I assume, code don't often happen by accident), it is depended upon by users, and is missing here from this one, single version of Yii Bootstrap extension.

Yii 2 users are pretty used to the way previous versions of widgets work and we should made it easier for them to move to this version.

... Is exactly what I've been saying all along. Not those exact words, maybe — sorry, English is not my first language.

At no point did I intend for this to get so drawn out, trust me — I was hoping for the first response comment of the issue to be something like "oh, right, we missed this/didn't think of that; sure we're gonna fix this".

I would even have missed this bug (or call it whatever you want) myself, if I didn't have a project based on one of the Yii 2 application templates that had that extended Alert widget, the one using session flash messages, covered with waterproof tests. Upgrading the project to this extension, the test case that verifies that those extended alerts can have customized close buttons failed.

The most ridiculous part is that I've never actually used customized alert close buttons for anything. Mostly that may be because I'm a systems/back-end engineer, and I sincerely could not give a damn less about how something appears as long as it does not get in the way of how something (or someone) works.

samdark commented

Interesting. We need to re-check how https://github.com/yiisoft/yii-bootstrap5 actually works in this case. It's not released yet so could contain bugs.

  • 👍1
simialbi commented

At no point did I intend for this to get so drawn out, trust me — I was hoping for the first response comment of the issue to be something like "oh, right, we missed this/didn't think of that; sure we're gonna fix this".

I understand what you mean. If it would be documented, or having the same behaviour as in previous versions, this case got never opened.

ISTM I have failed to present the point of the test. Here, I'll try to explain: the test verifies a single, very specific feature, behavior that is shared by pretty much all versions of Yii Bootstrap extension, previous and upcoming, this one being the odd one out.

In previous versions of bootstrap, the "icon" always got rendered by content: bs2, bs3, bs4. This is the first one who changed this behavior, why I changed it too.
But I understand your point.
My proposal: bring back the "label" option and explicitly mark it as deprecated in docs and add some hints about how to use it (maybe with @antichris' example).
@bizley, @samdark, @antichris: what do you think about? Can everyone live with it?

  • 👍1
bizley commented

I think that we should have it and it should not be deprecated. It will allow more customization like @antichris mentioned.

  • 👍1
samdark commented

I think that we should check yiisoft/yii-bootstrap5#53 first.

antichris commented

Given

Alert::widget([
    'body' => "Low Blow: Bob Loblaw's Law Blog Lobs Law Bomb",
    'options' => ['class' => 'alert-warning'],
    'closeButton' => [
        'label' => 'Dismiss',
        'tag' => 'a',
        'class' => ['buttonOptions' => 'btn btn-outline-warning'],
        'style' => 'position:absolute;top:.5rem;right:.5rem',
    ]
]);

yields

<div id="w0" class="alert-warning alert alert-dismissible" role="alert">

Low Blow: Bob Loblaw's Law Blog Lobs Law Bomb
<a type="button" class="btn btn-outline-warning" data-bs-dismiss="alert" aria-label="Close" label="Dismiss" style="position:absolute;top:.5rem;right:.5rem"></a>

</div>

image

The funky whitespace is consistent with previous versions, the <a> tag's attribute type="button" and lack of content is not. Compare with yiisoft/yii-bootstrap5#53 (comment).

samdark commented

Alright. Let's have it:

  1. To ease transition.
  2. It's useful overall.
  • 👍1