backdrop-contrib / ubercart

A flexible but easy-to-use e-commerce system for Backdrop.
https://backdropcms.org/project/ubercart
GNU General Public License v2.0
4 stars 10 forks source link

Deprecated function: Using ${var} in strings is deprecated, use {$var} in PHP 8.2 #470

Closed argiepiano closed 12 months ago

argiepiano commented 12 months ago

I'm getting this deprecation notice because of a "dollar sign outside curly braces" pattern This is happening in uc_store_rfc2822_display_name().

I believe the solution is to just use normal concatenation. The suggested fix in the PHP 8.2 page above may also work, but I'm a bit uneasy about this in this context (this is happening in a preg_match). At the very least, this pattern makes the regex even harder to understand.

argiepiano commented 12 months ago

It turns out that the suggested change here works just fine both in PHP 8.1 and PHP 8.2. PR ready for review.

To test, use PHP 8.2 and clear caches. Also, to test functionality, enable Rules UI, visit admin/store/settings/store and enter a store name that contains quotation marks. Then visit admin/config/workflow/rules/reaction/manage/uc_checkout_customer_notification/edit/3 and check that the " have been escaped as \".

bugfolder commented 12 months ago

I went to test this, starting with trying to reproduce the problem. Turned on PHP 8.2.0, entered a store name My Backdrop ""Site, then on the Rules action edit page I see the Sender name value "My Backdrop \"\"Site" <...>, which is already escaped. But I'm not seeing any deprecation notices. Making the change in this PR doesn't seem to affect anything. Am I missing something?

argiepiano commented 12 months ago

Hi @bugfolder. Before the change, with PHP 8.2 you get this when you clear caches:

Screen Shot 2023-11-26 at 4 14 30 PM

After the change there is no deprecation notice. Are you not seeing this in PHP 8.2?

argiepiano commented 12 months ago

So, the code was working fine before and after. The problem was not the result. The problem I fixed was the deprecation notice in PHP 8.2. The "testing steps" above were just to show that the functionality still worked as expected after the change.

bugfolder commented 12 months ago

I wasn't seeing the deprecation notice, even though I had PHP 8.2. But then I noticed that my contrib module test rig was only on 1.26.0, and so I installed 1.26.1, and poof, deprecation notice appears.

bugfolder commented 12 months ago

Seems I spoke too soon. I definitely saw the deprecation notice you quoted when I went to run the update.php from 1.26.0 to 1.26.1, but now, when I clear caches, I don't see any deprecation notice:

image

And I do still have my Store set to My Backdrop ""Site.

bugfolder commented 12 months ago

Ah, but going to update.php brings up the notice, even though there aren't any updates to run:

image

bugfolder commented 12 months ago

I can confirm that the notice is repeatable on the update.php screen, and the PR removes the notice from the update.php screen. So, WFM.

argiepiano commented 12 months ago

Thanks! Merging...