Closed chrispenny closed 1 year ago
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
@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.
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
Saweeet, thanks for the suggestions. I should be able to pick this up in the coming days.
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
.
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).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?
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.
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!
Sounds like this is a non-issue. Closing.
https://wiki.php.net/rfc/deprecate_dynamic_properties
I've been struggling a bit to understand how/where
jobData
is determined, but from manual testing it seems that it relies on us adding dynamic properties(?). EG: If I add a dynamic property to myJob
class, then whatever value is saved in that property will persist in theSavedJobData
, but as soon as I define that property on myJob
class, it seems to no longer get persisted inSavedJobData
.Does anyone have any thoughts on what the path forwards is, or perhaps I'm just missing something in the way that I implement my
Job
classes?Generally speaking, they go something like this..
Defined properties:
Dynamic properties: