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

Explicitly cast types in CRUDController::batchAction() #6529

Merged
merged 1 commit into from
Nov 26, 2020

Conversation

peter-gribanov
Copy link
Contributor

@peter-gribanov peter-gribanov commented Oct 23, 2020

Subject

This PR corrects several issues at once.

No data.all_elements in tpl

Key "all_elements" for array with keys "idx, action, won" does not exist.

Error in SonataAdminBundle:CRUD:batch_confirmation.html.twig

<div class="box-body">
    {% if data.all_elements %} {# at this line #}
        {{ 'message_batch_all_confirmation'|trans({}, 'SonataAdminBundle') }}
    {% else %}
        {% trans with {'%count%': data.idx|length} from 'SonataAdminBundle' %}message_batch_confirmation{% endtrans %}
    {% endif %}
</div>

The problem is due to the fact that the all_elements is a checkbox and if it is not checked, then it will not be in the HTTP request.

Form Data:

_sonata_csrf_token: rLjGreGh3zUGRjBSjebfU6jFjTuzbBhD0dCrs6RWn_g
idx[]: 441
idx[]: 440
idx[]: 439
idx[]: 438
idx[]: 437
action: approve

Undefined index: all_elements

Notice: Undefined index: all_elements

For the same reason, the all_elements will be absent in the data after confirming the action.

Form Data:

confirmation: ok
data: {"idx":["441","440","439","438","437"],"action":"approve"}
_sonata_csrf_token: rLjGreGh3zUGRjBSjebfU6jFjTuzbBhD0dCrs6RWn_g

TypeError

If you select apply to all elements, the ids is missing from the data.

TypeError: Argument 3 passed to Sonata\AdminBundle\Admin\AbstractAdmin::preBatchAction() must be of the type array, null given, called in /www/vendor/sonata-project/admin-bundle/src/Controller/CRUDController.php on line 517
#5 /vendor/sonata-project/admin-bundle/src/Admin\AbstractAdmin.php(805): Sonata\AdminBundle\Admin\AbstractAdmin::preBatchAction
#4 /vendor/sonata-project/admin-bundle/src/Controller/CRUDController.php(517): Sonata\AdminBundle\Controller\CRUDController::batchAction
#3 /vendor/symfony/http-kernel/HttpKernel.php(158): Symfony\Component\HttpKernel\HttpKernel::handleRaw
#2 /vendor/symfony/http-kernel/HttpKernel.php(80): Symfony\Component\HttpKernel\HttpKernel::handle
#1 /vendor/symfony/http-kernel/Kernel.php(201): Symfony\Component\HttpKernel\Kernel::handle
#0 /web/index.php(53): null

Form Data:

confirmation: ok
data: {"all_elements":"on","action":"approve"}
_sonata_csrf_token: 07f1crI4LnSge9E17Q08bMZb8isUsVkZXqUS5UFsNwM

Changelog

### Fixed
- Explicitly cast types in `CRUDController::batchAction()`

tests/Controller/CRUDControllerTest.php Outdated Show resolved Hide resolved
tests/Controller/CRUDControllerTest.php Outdated Show resolved Hide resolved
tests/Controller/CRUDControllerTest.php Outdated Show resolved Hide resolved
tests/Controller/CRUDControllerTest.php Outdated Show resolved Hide resolved
tests/Controller/CRUDControllerTest.php Outdated Show resolved Hide resolved
@@ -3644,6 +3644,114 @@ public function testBatchActionWithConfirmation(): void
$this->assertSame('@SonataAdmin/CRUD/batch_confirmation.html.twig', $this->template);
}

/**
* NEXT_MAJOR: Remove this legacy group.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why the legacy group is here. Is there any instruction that will be resolved in the next major about these changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two tests are copy/paste of CRUDControllerTest::testBatchActionWithConfirmation().

@peter-gribanov peter-gribanov force-pushed the batch_action branch 3 times, most recently from 22ad091 to bde79c0 Compare October 26, 2020 08:40
core23
core23 previously approved these changes Oct 31, 2020
phansys
phansys previously approved these changes Oct 31, 2020
@peter-gribanov
Copy link
Contributor Author

Any news?

@core23
Copy link
Member

core23 commented Nov 25, 2020

CI is stuck. Can you please rebase, so we can merge this

add tests

comment idx in tests

use data provider in tests
@peter-gribanov
Copy link
Contributor Author

CI is stuck. Can you please rebase, so we can merge this

@core23 It's done. PR is ready for marge.

@franmomu franmomu merged commit ddbc55b into sonata-project:3.x Nov 26, 2020
@franmomu
Copy link
Member

Thanks @peter-gribanov!

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.

4 participants