[Bug] 65672 - Allow DatePeriod extensions to have writable properties

[duncan3dc]

It looks like the properties were prevented from being writable as part of a serialization fix: 0ee7155

However there are no tests for this, so it seems it may have been unintentional. Also a bug was opened and hasn't been marked as "not a bug" so this PR restores the previous functionality, and includes tests for it.

[derickr]

Can this be used to write to DatePeriod native properties?

[duncan3dc]

According to the docs there aren't any public properties on the class, unless I've missed something?

[nikic]

As we now use delayed wakeup in unserialization the original reason for preventing writes to properties has gone away, so this LGTM.

[cmb69]

According to the docs there aren't any public properties on the class, unless I've missed something?

var_dump((new DatePeriod(new DateTime('now'), new DateInterval('P1D'), new DateTime('tomorrow')))->recurrences);

If it was possible to arbitrarily change these properties, I'd expect malfunctions.

[duncan3dc]

Thanks for that Christoph, I'll take a look at ensuring any public properties are prevented from being updated and that we have tests for them. I'll also update the docs with any public properties I find

[duncan3dc]

I've re-written this PR now, it changes the current functionality from preventing writes to any properties, to only prevent writes to native properties.

I've also added tests to ensure native properties can be read, to ensure they can't be written, and to ensure custom properties can be read and written.

I've also submit a documentation patch via the Docbook Online Editor to list these public properties

[imbrish]

@derickr @nikic any progress on this one?

[derickr]

I only had a question about functionality. I didn't look at the patch or implementation, so I defer to @nikic — however, Travis tests show failed for this PR.

[duncan3dc]

@derickr I believe the travis issue is a memory leak that was already present before this PR, and it looks like it has since been resolved by @nikic in b72b1a4

Should this PR target master/7.3 instead? Or is there a workaround for this memory leak we can apply to 7.1?

[duncan3dc]

I've rebased this against 7.3 to take advantage of the memory leak fix.
Travis shows ENABLE_MAINTAINER_ZTS=0 ENABLE_DEBUG=0 as passing, but the ENABLE_MAINTAINER_ZTS=1 ENABLE_DEBUG=1 build is still showing a memory leak (https://travis-ci.org/php/php-src/jobs/491390023).
Should I raise a bug for this separate issue?

[nikic]

@duncan3dc I've fixed the leak in a109fdd, please rebase again.

[nikic]

You'll also want to make similar modifications to date_period_read_property. The thing to test would be operations like $period->someArray[] = 123.

[duncan3dc]

@nikic Thanks for the help, I've updated now to handle the string properly, and ensure reading for modification is handled the same way

[nikic]

Looks good. Please be careful when merging up, I think in 7.4 you may need to change tests due to DateInterval comparison and in master you'll have to change the object handler signatures.

[nikic]

Do I remember right that you have php-src karma? If not, I can commit this change for you.

[duncan3dc]

@nikic I do, let me refactor those repeated property checks first though.
I'm on vacation this week, but I'll take a look next week then ping you for a final review.

Unless you'd prefer to get this merged sooner, in which case I'm happy for you to go ahead 👍

[duncan3dc]

I noticed an issue on PHP-7.4 while merging up, I've opened #4198 to demonstrate. I presume we'll want to get that resolved first before committing this change

[duncan3dc]

Committed as dc586bf and 8b53c72