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

Fix StatusClassRenderer implementation #92

Merged
merged 1 commit into from
Jun 27, 2020

Conversation

wbloszyk
Copy link
Member

Subject

After add type hints in Sonata\Twig\Extension\StatusRuntime get status class from string (flash massage type) is not possible and TypeError is throwed. This PR fix StatusClassRendererInterface implementation in FlashManager and remove some type hints which make BC-break.

I am targeting this branch, because this change respect BC.

Closes #89.

Changelog

### Fixed
- Fix `Sonata\Twig\Status\StatusClassRendererInterface` implementation in `Sonata\Twig\FlashMessage\FlashManager`
- Fix `Sonata\Twig\Extension\StatusRuntime` to working with `Sonata\Twig\FlashMessage\FlashManager` again, after add type hints

@wbloszyk
Copy link
Member Author

@phansys @core23 @VincentLanglet
I close #90 becouse it is hard to understand. I will split it to more atomic PR to better understand what is changing:

  • fix BC-break after add type hints - this PR
  • improve FlashManager - another RP

This PR is RTM. Can you check it?

VincentLanglet
VincentLanglet previously approved these changes Jun 24, 2020
@wbloszyk
Copy link
Member Author

@core23 @VincentLanglet @phansys @greg0ire Can you check it?

src/Extension/StatusRuntime.php Outdated Show resolved Hide resolved
src/FlashMessage/FlashManager.php Outdated Show resolved Hide resolved
@wbloszyk wbloszyk requested a review from phansys June 26, 2020 17:04
@wbloszyk wbloszyk requested a review from phansys June 26, 2020 18:58
src/Extension/StatusRuntime.php Outdated Show resolved Hide resolved
@greg0ire greg0ire added the patch label Jun 26, 2020
src/Extension/StatusRuntime.php Outdated Show resolved Hide resolved
src/Extension/StatusRuntime.php Outdated Show resolved Hide resolved
@wbloszyk wbloszyk force-pushed the fix_status_renderer_using branch from dc152d6 to f83d4be Compare June 26, 2020 20:32
@wbloszyk wbloszyk requested review from phansys and greg0ire June 26, 2020 20:37
greg0ire
greg0ire previously approved these changes Jun 26, 2020
@wbloszyk wbloszyk force-pushed the fix_status_renderer_using branch 3 times, most recently from 28c8ce4 to cb4a9a6 Compare June 27, 2020 07:02
@wbloszyk wbloszyk requested review from greg0ire and core23 June 27, 2020 07:16
tests/Extension/StatusRuntimeTest.php Outdated Show resolved Hide resolved
tests/Extension/StatusRuntimeTest.php Outdated Show resolved Hide resolved
Calling methods `handlesObject` and `getStatusClass` from `Sonata\Twig\FlashMessage\FlashManager`
throw ErrorType exception after add type hints in `Sonata\Twig\Status\StatusClassRendererInterface`
API.

Update src/Extension/StatusRuntime.php

Co-authored-by: Javier Spagnoletti <phansys@gmail.com>

Update src/FlashMessage/FlashManager.php

Co-authored-by: Javier Spagnoletti <phansys@gmail.com>

Update src/Extension/StatusRuntime.php

Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>

Update src/Extension/StatusRuntime.php

Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>

Update tests/Extension/StatusRuntimeTest.php

Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>

Update tests/Extension/StatusRuntimeTest.php

Co-authored-by: Grégoire Paris <postmaster@greg0ire.fr>
greg0ire
greg0ire previously approved these changes Jun 27, 2020
@wbloszyk wbloszyk force-pushed the fix_status_renderer_using branch from 7014b3f to e95d5c7 Compare June 27, 2020 08:02
@phansys phansys merged commit 4c60f33 into sonata-project:1.x Jun 27, 2020
@phansys
Copy link
Member

phansys commented Jun 27, 2020

Thank you @wbloszyk!

@benrcole
Copy link
Contributor

I cant be certain because I don't fully understand how the engine works but I think there's a problem with StatusRuntime::statusClassForFlashManager that's included within this change.

With this change my templates now render the flashes but the relevant css class is misssing for each flash type. I think it's down to the below code fragment. My suspicion is that the wrong parameter is being sent to handlesObject below.

 private function statusClassForFlashManager(string $object, ?string $statusType = null, string $default = ''): string
    {
        if ($flashManager = $this->getFlashManagerFromStatusServices()) {
            if ($flashManager->handlesObject($flashManager, $statusType)) {
                if (null === $statusType) {
                    $statusType = $object;
                }

Sending $object instead of $statusType as below makes the templates render correctly

    private function statusClassForFlashManager(string $object, ?string $statusType = null, string $default = ''): string
    {
        if ($flashManager = $this->getFlashManagerFromStatusServices()) {
            if ($flashManager->handlesObject($flashManager, $object)) {
                if (null === $statusType) {
                    $statusType = $object;
                }
...

Is this a correct fix or am I missing some config somewhere else?

@wbloszyk
Copy link
Member Author

wbloszyk commented Jun 28, 2020

@benrcole Look at https://github.com/sonata-project/twig-extensions/blob/1.x/docs/reference/status_helper.rst
and https://github.com/sonata-project/ecommerce/blob/3.x/src/Component/Order/OrderStatusRenderer.php
Change in remove src/FlashMessage/FlashManager.php fit it for correct way.

Can you open issue for this with reproduction information?

Im also working on #90 where using StatusClassRenderer and FlashManager will be more obvius.

@benrcole
Copy link
Contributor

I have opened #93 with reproducible steps

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

Successfully merging this pull request may close these issues.

Using flashmessage type based on string make error with extensions 1.x
6 participants