Moving dynamic properties into the private additionalProperties array breaks some existing application code

[room34]

• PHP Version: 8.2
• PHP date.timezone: America/Chicago
• ICS Parser Version: 3.2.1
• Linux

Description of the Issue:

This pertains to issue #316, which moved all dynamic properties of the Event object into the private additionalProperties 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

[u01jmg3]

Thanks for the detailed summary.

I guess we have a few choices.

1. Stick with what we have and create an upgrade guide similar to what @room34 has described
2. Revert #316 and instead use the #[AllowDynamicProperties] comment. We could then later release #316 as a major version bump to the library.
3. ?

@s0600204: what do you think?

[s0600204]

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.)

[room34]

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.

[u01jmg3]

Thanks @s0600204

Todo:

• Create a magic __isset() magic method on the Event class

[u01jmg3]

@s0600204, @room34: see b01d72b