Adding URL redirects from node form creates warning on log messages

[sudipto68]

Description of the bug

Getting this Warning when adding a redirect to my fresh backdrop site. The redirect works perfectly but it creates a warning message related to core/modules/redirect/redirect.admin.inc file.

Steps To Reproduce:

1. Create a test node.
2. Edit the “test” node and add a redirect to any other node.

Then save redirect and now check the recent log messages and you should see now a warning like this count(): Parameter must be an array or an object that implements Countable in redirect_edit_form()

Additional information

• Backdrop CMS version: 1.23.1
• Web server and its version: Nginx/1.18.0
• Operating System and its version: Ubuntu 20.04 lts
• Browser(s) and their versions: Google Chrome v107.0.5304.87

[indigoxela]

@sudipto68 many thanks for reporting this bug.

I can confirm it, and - even worse - on PHP 8 it's a fatal one.

Two observations:

One: count(form_get_errors()) assumes that form_get_errors() always returns something, that's countable. In fact, form_get_errors() can return NULL. This gives a warning with php 7.x, but a fatal error with php 8.1.

Two: I didn't find any constellation, where form_get_errors() in function redirect_edit_form is not NULL.

The easiest solution would be type casting, but I'd like to understand, whether the form_get_errors() check will ever work as expected at that point.

[argiepiano]

I didn't find any constellation, where form_get_errors() in function redirect_edit_form is not NULL.

At quick glance, I see that redirect_element_validate_source() can add an error to the form. This function is used to validate the source element in the form, and it forces a form rebuild. So it seems like redirect_edit_form() will get that error when being rebuilt.

[indigoxela]

@argiepiano many thanks for your check. :pray: But ... if I make it a loop (redirect to same, clearly an error), validation fails - and still form_get_errors() returns NULL. Any other wrong use I could try?

Edit: fun fact, if I make the source node/0000000000000, the value just saves :stuck_out_tongue:

[indigoxela]

Gosh...

From: http:// (literally!) To: http://

Message: The source path is an existing path.... blabla...

Additionally lots of Undefined array key "path" and form_get_errors() is still NULL - heck, what does that form validator do, anyway?

[klonos]

Sounds like we should be using empty() instead of count() ...either that, or a combination of is_array() && empty() 🤔 ...plus also fix those weird things that @indigoxela found with those edge cases 😅

Any other wrong use I could try? 🤣 👍🏼

[herbdool]

There are a number of places in core that just use !form_get_errors() so I've done that in my PR.

[klonos]

Yup, the code change is simple, and no errors logged when creating redirects (plus all tests pass) 👍🏼 ...given that we are doing this elsewhere, I'm marking this as WFM + RTBC.

Thanks @herbdool 🙏🏼

[quicksketch]

Thanks everyone! I merged https://github.com/backdrop/backdrop/pull/4372 into 1.x and 1.24.x. Extra thanks to @sudipto68 for reporting this issue and with such detail!

https://github.com/backdrop/backdrop/commit/f695ee8717361df24083e5080e438abed95914eb by @herbdool, @indigoxela, @sudipto68, @klonos, and @argiepiano.