zencart / zencart

Zen Cart® is a full-function e-commerce application for your website.
https://github.com/zencart/zencart/releases
Other
375 stars 233 forks source link

PHP8.2 Customization New Fields Cause Dynamic Properties Deprecated #5161

Closed brittainmark closed 1 year ago

brittainmark commented 2 years ago

Describe the bug If someone writes a plugin that has fields that needs to be displayed/edited. Then the field name needs to be added to the declarations in object_info.php this would be true for any database changes or anywhere that object_info is used to set up the class with the appropriate data.

Version Zen Cart version: 1.5.8 PHP version: 8.2

To Reproduce Steps to reproduce/demonstrate the behavior:

  1. Go to create plugin that has extra data field
  2. install plugin
  3. run screen that uses plugin 4.depreciated log produced

Expected behavior no log file

Debug/Error Logs

[23-Aug-2022 11:03:00 UTC] Request URI: /Test158/Admin/index.php?cmd=category_product_listing&page=0&cPath=4, IP address: 127.0.0.1
#0 /home/lotus/public_html/innerlightcrystals/Test158/Admin/includes/classes/object_info.php(701): zen_debug_error_handler()
#1 /home/lotus/public_html/innerlightcrystals/Test158/Admin/includes/classes/object_info.php(690): objectInfo->__set()
#2 /home/lotus/public_html/innerlightcrystals/Test158/Admin/includes/classes/object_info.php(666): objectInfo->updateObjectInfo()
#3 /home/lotus/public_html/innerlightcrystals/Test158/Admin/category_product_listing.php(850): objectInfo->__construct()
#4 /home/lotus/public_html/innerlightcrystals/Test158/Admin/index.php(11): require('...')
--> PHP Deprecated: Creation of dynamic property objectInfo::$vs_products_location is deprecated in /home/lotus/public_html/innerlightcrystals/Test158/Admin/includes/classes/object_info.php on line 701.

This is for a plug-in i have been working on to display a product location. vs_products_location is the variable that is displayed on screen.

Additional context this could affect all plug-ins and how they work. Sorry, at this point I have no solution. Not sure if it will require a major rethink of how plugins work or how class objectInfo is used/works.

scottcwilson commented 2 years ago

I wonder in this case if we can use #[AllowDynamicProperties].

https://wiki.php.net/rfc/deprecate_dynamic_properties

brittainmark commented 2 years ago

Yes looks like an option. Might need it on any pages that have observers that allow additional data to be added.

scottcwilson commented 2 years ago

Payment Modules seem like the perfect place for #[AllowDynamicProperties] to allow us to wrap this up.

scottcwilson commented 2 years ago

Plugin developers will need to extend objectInfo to handle new database fields not covered by the base class.

https://docs.zen-cart.com/dev/plugins/upgrading_to_158/#php-82-objectinfo-and-plugins

brittainmark commented 2 years ago

For this to work would have to add notifiers to allow objectinfo to be replaced with the extended class. Otherwise, you will require changes to the core files.

For example Admin/category_product_listing.php around line 850

if ((!isset($_GET['pID']) && !isset($_GET['cID']) || (isset($_GET['pID']) && ($_GET['pID'] === $product['products_id']))) && !isset($pInfo) && !isset($cInfo) && (strpos($action, 'new')
                      !== 0)) {
                $pInfo = new objectInfo($product);
              }

add notifier to allow objectInfo to be replaced with your extended class something like this

if ((!isset($_GET['pID']) && !isset($_GET['cID']) || (isset($_GET['pID']) && ($_GET['pID'] === $product['products_id']))) && !isset($pInfo) && !isset($cInfo) && (strpos($action, 'new')
                      !== 0)) {
                $object_info = objectinfo
                $zco_notifier->notify('NOTIFY_ADMIN_CATEGORY_PRODUCT_OBJECTINFO', $pInfo,$object_info);
                $pInfo = new $object_info($product);
              }

This would need to be done for every occurrence of "new oblectInfo" where additional fields were being added to $_POST

This will be a lot of new notifications.

Don't know if it will work as I have not tested it. Also, not sure if I have the syntax correct. But hopefully you get the idea.

Additional, how do we cope with multiple addins using the same object.

Would you have to declare the class by adding a bit to the object name

class Myaddin.$object_info extends $object_info

so that each one builds on the previous one.

Don't know if this is possible?

scottcwilson commented 2 years ago

I don't think I'm tracking what you're doing here. Please put your plugin together (and use a name more obvious than object_info for the new class) and we'll figure out what we need to do to get it working.

scottcwilson commented 2 years ago

We may want to handle plugins (or even regular objectInfo object creations) using the __get() and __set() magic methods instead of enumerating every possible data element. That might work too.

brittainmark commented 2 years ago

In this case I was adding a table product location (products_id,products_location) that I wanted to include on the invoice and packing slip and also needed to enter into the products screen.

The issue I get is the fields collected by collect_info create $_POST entries. These are passed to object_info class which tries to create the parameter and then the warning.

You can see what I am trying to do here, product location Note this is a work in progress.

What I was trying to do above was dynamically create the class names. So that if more than one plugin used the same notifier, it could create its class using the previous class. Thus, first plug in class myFirstPlugin extends objectInfo, second plugin class mySecondPlugin extend myFirstPlugin thus chaining the class extensions to keep all the declared parameters.

I am no expert in PHP, so I don't know if this is possible.

scottcwilson commented 2 years ago

What I was trying to do above was dynamically create the class names. So that if more than one plugin used the same notifier, it could create its class using the previous class.

Don't do this. You're making it too complicated. Just handle your own work. Someone else will do what they need to do.

brittainmark commented 2 years ago

Understand. In that case, I will need #[AllowDynamicProperties] for objectIinfo unless magic __get and __set work. I assume objectInfo will need recoding to use the magic__get and __set for the $_POST variables. Will leave all alone for now! Let someone who understands it better than me look at it.

scottcwilson commented 2 years ago

Can you find a plugin which is already working that does something roughly similar to what you're doing? I can upgrade that one and you can see how I did it. It's tricky to debug two issues at once.

brittainmark commented 2 years ago

I have it working except for ez-populate on my machine, The only issue is the warning. I have put a working version here without ez-populate.

scottcwilson commented 2 years ago

Update your repo - I just changed the base class to allow dynamic properties. For this particular object, it seems a reasonable choice.

brittainmark commented 2 years ago

Understand the words. Not sure what has to be changed. Are you saying that we add #[AllowDynamicProperties] to objectInfo?

scottcwilson commented 2 years ago

It's already done. Do a git pull and retest your plugin.

brittainmark commented 2 years ago

Sorry, did not refresh my git hub view. Yes, looks a lot cleaner without all the defines. You will have to change your example in the documentation.

scottcwilson commented 2 years ago

I am noodling on it - I want to encourage people to not use #[AllowDynamicProperties] IF the number of properties is small and reasonable. Working on better wording.

scottcwilson commented 2 years ago

Updated the doc: https://docs.zen-cart.com/dev/plugins/upgrading_to_158/

brittainmark commented 2 years ago

I have had a look at magic method __set. If you have

public function __set($field, $value)
    {
        $this->$field = $value;
    }

it still shows the depreciation message. However, if you have

public function __set($field, $value)
    {
    }

the message is not displayed and works fine. I change objectInfo removing #[AllowDynamicProperties] and my plugin worked without complaint.

Just thought I would let you know.

scottcwilson commented 2 years ago

If your set function is empty, are you sure your data is being stored and that your software actually works?

brittainmark commented 2 years ago

Yes I made changes to my field and all worked fine. got the idea from here

scottcwilson commented 2 years ago

Yep, it says

Note that setting a dynamic property from within the __set method is still deprecated: This is a good clarification; it's different than I thought it was.

brittainmark commented 2 years ago

Think I have found another class that may need magic set. includes/classes/navigation_history.php: it includes the function unserialize

 public function unserialize($broken)
    {
        foreach ($broken as $kv) {
            $key = $kv['key'];
            if (gettype($this->$key) !== 'user function') {
                $this->$key = $kv['value'];
            }
        }
    }

This used $key to define class parameters. I cannot find any use of the function in Zen cart, so another option would be to remove the function from the class.