Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Property hooks #82

Closed
wants to merge 71 commits into from
Closed

Property hooks #82

wants to merge 71 commits into from

Conversation

iluuu1994
Copy link
Owner

@iluuu1994 iluuu1994 commented May 1, 2023

No description provided.

iluuu1994 and others added 17 commits May 24, 2023 20:45
{h,c} is not actually a supported pattern
* PHP-8.2:
  [skip ci] Fix release date of PHP 8.2.7
* PHP-8.1:
  Access violation when ALLOC_FALLBACK fixed
* PHP-8.2:
  Access violation when ALLOC_FALLBACK fixed
…ceptions and segfaults with replaceWith

This replaces the implementation of before and after with one following
the spec very strictly, instead of trying to figure out the state we're
in by looking at the pointers. Also relaxes the condition on text node
copying to prevent working on a stale node pointer.

Closes phpGH-11299.
* PHP-8.1:
  Fix phpGH-11288 and phpGH-11289 and phpGH-11290 and phpGH-9142: DOMExceptions and segfaults with replaceWith
* PHP-8.2:
  Fix phpGH-11288 and phpGH-11289 and phpGH-11290 and phpGH-9142: DOMExceptions and segfaults with replaceWith
…memory=1

The function repeatedly calls mprotect() which is extremely slow. In our
community build, the Laravel tests went from ~6 minutes to ~4 hours. This issue
only occurs with opcache.protect_memory=1.

Closes phpGH-11323
* PHP-8.2:
  Fix zend_jit_stop_counter_handlers() performance issues with protect_memory=1
The var_dump can be preceded by the "Interactive shell" log. The var_dump does
not add much to the test anyway, so just remove it.
* PHP-8.1:
  [skip ci] Fix race condition in readline test
* PHP-8.2:
  [skip ci] Fix race condition in readline test
Array literals will constant evaluate their elements. These can include
assignments, even though these are not valid constant expressions. The lhs of
assignments can be a list() element (or []) which is parsed as an array with a
special flag.
Zend/zend_property_hooks.c Outdated Show resolved Hide resolved
nielsdos and others added 6 commits June 4, 2023 16:19
…le namespaces

The test was amended from the original issue report. For the test:
Co-authored-by: php@deep-freeze.ca

The problem is that the regular dom_reconcile_ns() only works on a
single node. We actually have to reconciliate the whole tree in case a
fragment was added. This also required to move some code around such
that this special case could be handled separately.

Closes phpGH-11362.
From the moment an ID is created, libxml2's behaviour is to cache that element,
even if that element is not yet attached to the document. Similarly, only upon
destruction of the element the ID is actually removed by libxml2.
Since libxml2 has such behaviour deeply ingrained in the library, and uses the
cache for various purposes, it seems like a bad idea and lost cause to fight it.
Instead, we'll simply walk the tree upwards to check if the node is attached to
the document.

Closes phpGH-11369.
* PHP-8.1:
  Fix bug #77686: Removed elements are still returned by getElementById
  Fix bug #81642: DOMChildNode::replaceWith() bug when replacing a node with itself
  Fix bug #67440: append_node of a DOMDocumentFragment does not reconcile namespaces
* PHP-8.2:
  Fix bug #77686: Removed elements are still returned by getElementById
  Fix bug #81642: DOMChildNode::replaceWith() bug when replacing a node with itself
  Fix bug #67440: append_node of a DOMDocumentFragment does not reconcile namespaces
Co-authored-by: Nikita Popov <nikita.ppv@gmail.com>
@ikkez
Copy link

ikkez commented Jun 15, 2023

Hi @iluuu1994
I have no idea how to get in contact and reach you through that php mailing list, but I am following the Property Hooks and the old property accessor RFC for feature a while now.

The base problem these RFCs try to solve is that we cannot intercept a public property's access, while it would be so useful. I'm here with you, but I'm thinking about a different solution that might just solve that problem in an easier way than hooks. If we'd just have a property flag to leave the public property uninitialized by default, we could fallback to the magic get/set method for it's access.
It's also possible right now, I've put it in here to show you what I mean https://3v4l.org/fQ4AG#v8.2.7
What do you think about that? Probably it's less type save by now, but maybe there's a solution for this somewhere. It would just make things so much easier to use and adapt for projects I guess.
kind regards, CK

@KapitanOczywisty
Copy link

The base problem these RFCs try to solve is that we cannot intercept a public property's access

And asymmetric visibility, and on-set validation, and virtual properties, and allow changes in logic without breaking API, and likely more.

I'm thinking about a different solution that might just solve that problem in an easier way than hooks

This would add new keyword for relatively low gain of single use case.

@JoyceBabu
Copy link

I am very much interested in this RFC and hope it will pass. But does this require reserving the keywords get and set? That would break every application depending on PSR-11.

@iluuu1994
Copy link
Owner Author

get/set aren't keywords in this RFC, functions can continue using that name.

@shaedrich
Copy link

shaedrich commented Sep 14, 2023

Is there a technical reason for throwing error for a redundant final keyword on both the property and the hook? I mean, would that be any harm? If not, I suspect, an E_NOTICE would suffice, wouldn't it?

Would it make sense, to add a jsonSerialize hook to the Future scope for get hooks that don't return a scalar value?

@ghost
Copy link

ghost commented Dec 2, 2023

Hi, is it possible to merge upstream master and resolve conflicts?? I would like to build PHP from this branch and play around with this feature.

@iluuu1994
Copy link
Owner Author

@javaDeveloperKid This branch is a bit outdated but functional. You can just check it out and build it like normal.

@iluuu1994 iluuu1994 closed this Feb 1, 2024
@SerafimArts
Copy link

SerafimArts commented Feb 4, 2024

closed this 3 days ago

I apologize, perhaps I did not see the necessary discussion in internals/externals, but do I understand correctly that work on adding properties to the php has been cancelled? =(

Frankly, adding property support was one of the most desirable possible future innovations in the PHP.next.

P.S. Whoops, didnt notice #122

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment