Problem/Motivation

BlockClassConfirmBulkOperationForm::getQuestion() returns $message_to_confirm, but the variable is only assigned inside the switch ($this->operation) branches and is never initialised. The block_class.confirm_bulk_operation route takes {operation} as a free URL parameter, so any value that matches no case reaches the return with the variable unset.

Steps to reproduce

  • Install Block Class 4.0.x-dev.
  • As a user with the administer block classes permission, visit:
/admin/config/content/block-class/confirm-bulk-operation/bogus_op/x/x/x/x/x/x

The page returns HTTP 200 and renders Warning: Undefined variable $message_to_confirm.

Proposed resolution

Initialise the variable before the switch, as already proposed in MR !5 on the closed issue #3279758:

public function getQuestion() {
  $message_to_confirm = "";

  switch ($this->operation) {

Worth considering as part of the same change: the insert case falls through without assigning a message, so a legitimate insert operation also returns an empty question.

Remaining tasks

  • Open a merge request against 4.0.x, crediting hswong3i for the original change in MR !5.
  • Review and test.

User interface changes

None, other than the PHP warning no longer being rendered on the confirmation page.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#6 3619190 after.png129.74 KBcsakiistvan
#6 3619190 before.png309.87 KBcsakiistvan

Issue fork block_class-3619190

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

csakiistvan created an issue. See original summary.

akhilsoni made their first commit to this issue’s fork.

akhilsoni opened merge request !87

akhilsoni’s picture

akhilsoni commented
Issue summary: View changes
Status: Active » Needs review

I have created a merge request with the proposed fix.

Initialised $message_to_confirm before the switch statement in BlockClassConfirmBulkOperationForm::getQuestion() to prevent the undefined variable warning. I've also added a functional test to cover the invalid operation scenario.

Coding standards pass cleanly with PHPCS. Please review.

csakiistvan’s picture

csakiistvan
Primary language Hungarian
Location Budapest, Hungary
commented
Assigned: Unassigned » csakiistvan
csakiistvan’s picture

csakiistvan
Primary language Hungarian
Location Budapest, Hungary
commented
Assigned: csakiistvan » Unassigned
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new 3619190 before.png309.87 KB
new 3619190 after.png129.74 KB

✅ Tested and works — reproduced the Undefined variable $message_to_confirm warning on 4.0.x at /admin/config/content/block-class/confirm-bulk-operation/bogus_op/x/x/x/x/x/x, then applied MR !87, rebuilt caches and reloaded the page: the warning is gone. The fix resolves the reported warning.

dydave’s picture

dydave commented

Thanks a lot everyone for the great work! 🙏

I'll do a pass on module's 4.x issues, this week and the following.

More to come very soon ... 😊

Thanks again!

dydave’s picture

dydave commented

Great job everyone!! Thanks a lot!! 🙏

Akhil (@akhilsoni), great documentation job of the issue and the merge request looks great. 🤩
Thanks a lot for the added phpunit test case... that's super helpful!

Indeed, I've got the following warning appearing in my VSCode:

Possible undefined variable '$message_to_confirm'.intelephense(P1116)
@var \Drupal\Core\StringTranslation\TranslatableMarkup|unset $message_to_confirm

So these changes should definitely be accepted.

However, I think you've pointed out another more annoying issue:

Why does the following URL not return a 404? 😅
/admin/config/content/block-class/confirm-bulk-operation/bogus_op/x/x/x/x/x/x

The bogus_op operation does not exist, so this page actually does not make any sense and there is currently no way to navigate to this page though the interface....

It probably slipped through the code cracks and since submitting the form on this page does nothing, I assume ... it just does no harm (?!). 😅

Anyway... this should probably be the object of a follow-up issue.

Now, I'll get to the code review, merging, and moving on...

  • dydave committed 9ea3f1ef on 4.0.x authored by akhilsoni 
    Issue #3619190 by akhilsoni, csakiistvan, dydave: Fixed warning error...
dydave’s picture

dydave commented
Status: Reviewed & tested by the community » Fixed

OK, I tested this locally, made some minor changes to the tests by adding two more checks on the tested page.

Since all the tests were still passing green 🟢, I went ahead and merged the changes above at #9. 🥳

I created the follow-up task #3627167: Refactor BlockClassConfirmBulkOperationForm operations and pages to address the issue with the validation of module's route parameters, in particular the selected bulk operation.
Personally, I think "ideally" it would be great if the module could rely on a different module to perform bulk operations.
But that's part of a much wider scope and discussion.

Marking issue as Fixed for now.
Thanks again! 🙏

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.