Closed room34 closed 1 year ago
Thanks for the detailed summary.
I guess we have a few choices.
#[AllowDynamicProperties]
comment. We could then later release #316 as a major version bump to the library.@s0600204: what do you think?
I'm a little lost, frankly. I don't see the problem that room34 is having, nor what he means to "use the __get()
magic method instead" (emphasis mine).
For instance, using php 8.2.3 and the latest release of the ics-parser, I can run...
$ical = new ICal\ICal($filename);
foreach ($ical->events() as $event) {
echo $event->attach_array[1] . "\n";
}
...and get sensible output when run with an ics file with ATTACH
stanzas on events.
But then that's to be expected: the __get()
magic method gets called if the property requested doesn't exist so that additional handling may be performed. (And no: __get()
won't work with non-dynamically defined properties. But then it doesn't need to.)
At no point would I expect the __get()
method to be called explicitly by an end user (e.g. $event->__get('summary_array')
).
I will note one change in behaviour that may not be desired: previously, a user could use either isset()
or property_exists()
to determine if a dynamic property existed, but didn't care for its value.
Now, both return False
.
However, if a magic __isset()
magic method is added to the Event
class, then isset($event->attach_array)
(for instance) could return True
where appropriate. (property_exists()
would still return False
.)
Ah, thanks for this clarification… I think you've hit on what was my real issue (and I'm not quite sure how I didn't put it together before). I was using isset()
and that is apparently what was causing failures for me.
Suffice to say, this is an example of why I'm not submitting pull requests. I have my head a bit too buried in my own code to be able to contribute productively to this project.
Thanks @s0600204
Todo:
__isset()
magic method on the Event class@s0600204, @room34: see b01d72b
8.2
America/Chicago
3.2.1
Description of the Issue:
This pertains to issue #316, which moved all dynamic properties of the
Event
object into the privateadditionalProperties
array to resolve deprecation notices in PHP 8.2.Throughout my application I have always liberally accessed
Event
properties directly, e.g.$event->attach_array
, because that approach is used several times in the documentation, and there was not, to my knowledge, a preferred getter method.Many of these properties apparently were being dynamically defined, which creates deprecation notices in PHP 8.2. So the solution in #316 was to move them into a defined
additionalProperties
array, and access them using the magic__get()
method.So far so good, but this resulted in a handful of mysterious "disappearances" of data for some of my users, and the resolution requires scouring my code for any instances of a property that might be dynamically defined, and changing those to use the
__get()
method instead. (Does__get()
also work for the defined properties? I suppose it does, but I haven't tested it, because I'm just putting out fires right now.)Apparently this situation could have been avoided, as it's possible to configure a class to allow dynamic properties, eliminating the deprecation notices in PHP 8.2:
https://php.watch/versions/8.2/dynamic-properties-deprecated#AllowDynamicProperties
I am not specifically requesting a change to this, as I have already made most if not all of the necessary changes in my code. I just wanted to bring this issue to contributors' attention.
Steps to Reproduce:
N/A