PHP 8.2: Dynamic properties deprecated, but are currently required for JobData? about silverstripe-queuedjobs HOT 8 CLOSED

OK, I see what's going on here, if we rely on dynamic properties we end up here https://github.com/symbiote/silverstripe-queuedjobs/blob/4/src/Services/AbstractQueuedJob.php#L256

    /**
     * Convenience methods for setting and getting job data
     *
     * @param mixed $name
     * @param mixed $value
     */
    public function __set($name, $value)
    {
        if (!$this->jobData) {
            $this->jobData = new stdClass();
        }
        $this->jobData->$name = $value;
    }

If we use regular properties, __set() is never called

Unfortunately the only other way to set jobData, is this, which has a bunch of parameters that we do not want

    /**
     * @param int $totalSteps
     * @param int $currentStep
     * @param boolean $isComplete
     * @param stdClass $jobData
     * @param array $messages
     */
    public function setJobData($totalSteps, $currentStep, $isComplete, $jobData, $messages)
    {
        $this->totalSteps = $totalSteps;
        $this->currentStep = $currentStep;
        $this->isComplete = $isComplete;
        $this->jobData = $jobData;
        $this->messages = $messages;
    }

In PHP 8.2, dynamic properties are deprecated so will throw deprecation errors, and will be removed in PHP 9 where they will throw hard exceptions

Option for PHP 8.2 support are:
a) Requires a new major release of queuedjobs with __set() and __get() removed.
b) Add the #[AllowDynamicProperties] class attribute to suppress the deprecation warning

I'm presuming b) will only work for PHP ^8.2 and will not work for PHP 9, so we'll need to implement a) regardless at some point

from silverstripe-queuedjobs.

setJobDataProperty() seems pretty reasonable. I assume you'll also need getJobDataProperty() as well?

If you're happy to do a PR here, please target at the 4 branch and also pop in some docblock deprecations on __get() and __set() and explain that dynamic properties will be removed in the next major version of queuedjobs - e.g. https://github.com/silverstripe/silverstripe-framework/blob/511b3bb060cb2962e79e6a034eea818b6c890ba4/src/ORM/ValidationResult.php#L235

Probably also a good time to add in the #[\AllowDynamicProperties] attribute to the AbstractQueuedJob class - I'm going to assume it not being present pre PHP8.2 won't actually matter since it starts with a hash # so it just renders as a comment

from silverstripe-queuedjobs.

Re-looking at this at part of the wider adding PHP 8.2 issue - this should be fine in PHP 8.2 and I don't think we even need the #[\AllowDynamicProperties]

AbstractQueuedJob already has a defined protected $jobData (stdClass)

stdClass DOES allow dynamic properities in PHP 8.2

Any setting of a dynamic properities of a job e.g. $job->lorem = 'ipsum'; will result in a call to AbstractQueuedJob::__set()

    public function __set($name, $value)
    {
        if (!$this->jobData) {
            $this->jobData = new stdClass();
        }
        $this->jobData->$name = $value;
    }

This method will still work in PHP 8.2 - see https://stitcher.io/blog/deprecated-dynamic-properties-in-php-82#implementing-__get-and-__set-still-works!

from silverstripe-queuedjobs.

@emteknetnz I think we still want all of the properties that are in the setJobData() method, as they are what is required for "steps"/etc. I think the main issue is that $this->jobData (I'm going to call this "additional job data") is set (somewhat) invisibly.

Perhaps there is an option c as well?

c) Add the #[AllowDynamicProperties] for BC until PHP 9, but also implement the "new" method for setting additional job data.

I'm thinking that it could probably be something very simple, like setJobDataProperty(string $name, mixed $value), which can just do the same as the __set() method?

Modules/projects that want to get ahead of the curve could start defining their properties (which will not get picked up by the current __set() method), and they could start persisting their additional job data through this new method.

from silverstripe-queuedjobs.

Saweeet, thanks for the suggestions. I should be able to pick this up in the coming days.

from silverstripe-queuedjobs.

I had a go at adding forward support (while keeping the status quo for BC), and I don't think it's possible for us to support both. Basically...

The module itself uses dynamic properties all over the place. This means that there could be code out there that is accessing those dynamic properties either to read or write information to/from jobData.

• If I switch our module's usages of those dynamic properties to the new setJobDataProperty() method, then that means that the (previously public) dynamic property will no longer exist, so if a dev has some existing code that is accessing that property to read its value, then it would now be null/undefined (as we have stopped defining it).
• If I define the property on our class (so that it's accessible to read, for the above scenario), then it will no longer automatically get updated in jobData if a developer is directly accessing that property in order to set its value.

It's like... either way, you make read or write non backwards compatible.

I think that this will need to be a breaking change whenever we decide to remove the dynamic properties. Which I guess we'll do for PHP 8.2?

from silverstripe-queuedjobs.

I think that this will need to be a breaking change whenever we decide to remove the dynamic properties. Which I guess we'll do for PHP 8.2?

Unless I’m missing something, we don’t need to remove dynamic properties. We can add the #[AllowDynamicProperties] annotation and put our feet up.

The RFC states that PHP 9.0 will throw an Error exception when setting dynamic properties on classes that aren’t marked with #[AllowDynamicProperties]. In fact the RFC also says:

Classes marked with #[AllowDynamicProperties] as well as their children can continue using dynamic properties without deprecation or removal.

from silverstripe-queuedjobs.

Sounds like this is a non-issue. Closing.

from silverstripe-queuedjobs.