Accessors v2 Patch

13 years ago by Clint M Priest — view source — reply

unread

https://bugs.php.net/patch-display.php?bug_id=49526&patch=accessor_v2.diff&revision=latest

I rewrote part of how it handles the job due to abysmal performance of version 1. This one is near the same performance as without the code changes using __get() and is actually faster in some cases (public getter).

I also checked all the tests and fixed the issues I found related.

There is however some strange behavior that I do not understand. For some reason if I run all of the tests, 100+ or so will fail. If I have the test script write those failed tests to a file and just run those tests, all but four work fine. It appears to be something to do with line endings, in other cases the output does not match expected because of the addition of 'unicode' to the output. I've no idea what's going on here but after 3 days of mucking around, I can't find a solution as to why this is occurring.

These failures do not occur on a fresh checkout but nothing I've changed should be causing any of the issues, just no idea here, any help would be appreciated.

Felipe: I could not find any segmentation fault tests from before when you posted, nor now. Are you still seeing segmentation faults?

Thoughts?

-Clint

13 years ago by Will Fitch — view source — reply

unread

Much better implementation.

As for the failed tests, I would double check and make sure you don't
have any stray debug statements (e.g. printf).

Sent from my iPhone

https://bugs.php.net/patch-display.php?bug_id=49526&patch=accessor_v2.diff&revision=latest

I rewrote part of how it handles the job due to abysmal performance of version 1. This one is near the same performance as without the code changes using __get() and is actually faster in some cases (public getter).

I also checked all the tests and fixed the issues I found related.

There is however some strange behavior that I do not understand. For some reason if I run all of the tests, 100+ or so will fail. If I have the test script write those failed tests to a file and just run those tests, all but four work fine. It appears to be something to do with line endings, in other cases the output does not match expected because of the addition of 'unicode' to the output. I've no idea what's going on here but after 3 days of mucking around, I can't find a solution as to why this is occurring.

These failures do not occur on a fresh checkout but nothing I've changed should be causing any of the issues, just no idea here, any help would be appreciated.

Felipe: I could not find any segmentation fault tests from before when you posted, nor now. Are you still seeing segmentation faults?

Thoughts?

-Clint

13 years ago by Clint M Priest — view source — reply

unread

https://bugs.php.net/patch-display.php?bug_id=49526&patch=v2.1&revision=latest

Alright, nevermind on the failing tests, figured that out. I didn't realize the test structure doesn't indicate that it was segfaulting, that fixed all of the failing tests.

So... I guess this patch is up for discussion?

I don't presently have any static getters implemented, as best I can tell it would require some changes to opcodes, I didn't dive into anything besides exploration there.

-Clint

-----Original Message-----
From: Will Fitch [mailto:will.fitch@gmail.com]
Sent: Sunday, December 11, 2011 1:41 PM
To: Clint M Priest
Cc: internals@lists.php.net
Subject: Re: [PHP-DEV] Accessors v2 Patch

Much better implementation.

As for the failed tests, I would double check and make sure you don't have any stray debug statements (e.g. printf).

Sent from my iPhone

https://bugs.php.net/patch-display.php?bug_id=49526&patch=accessor_v2.
diff&revision=latest

I rewrote part of how it handles the job due to abysmal performance of version 1. This one is near the same performance as without the code changes using __get() and is actually faster in some cases (public getter).

I also checked all the tests and fixed the issues I found related.

There is however some strange behavior that I do not understand. For some reason if I run all of the tests, 100+ or so will fail. If I have the test script write those failed tests to a file and just run those tests, all but four work fine. It appears to be something to do with line endings, in other cases the output does not match expected because of the addition of 'unicode' to the output. I've no idea what's going on here but after 3 days of mucking around, I can't find a solution as to why this is occurring.

These failures do not occur on a fresh checkout but nothing I've changed should be causing any of the issues, just no idea here, any help would be appreciated.

Felipe: I could not find any segmentation fault tests from before when you posted, nor now. Are you still seeing segmentation faults?

Thoughts?

-Clint

13 years ago by Pierre Joye — view source — reply

unread

Hi Clint,

Thanks for the updated patch :)

Some suggestions (did not test it yet only review it),

add test cases for non public property (private, protected)
show examples of the concept of readonly property (as it is allowed
in the C# equivalent

I would also suggest to either take over the RFC (contact the original
author) or create your own to update it with a full list of examples,
documentation, and all necessary information. This is a complex task
with many possible (edge) cases. An updated RFC is a must to be able
to discuss such additions.

Cheers,

https://bugs.php.net/patch-display.php?bug_id=49526&patch=v2.1&revision=latest

Alright, nevermind on the failing tests, figured that out. I didn't realize the test structure doesn't indicate that it was segfaulting, that fixed all of the failing tests.

So... I guess this patch is up for discussion?

I don't presently have any static getters implemented, as best I can tell it would require some changes to opcodes, I didn't dive into anything besides exploration there.

-Clint

-----Original Message-----
From: Will Fitch [mailto:will.fitch@gmail.com]
Sent: Sunday, December 11, 2011 1:41 PM
To: Clint M Priest
Cc: internals@lists.php.net
Subject: Re: [PHP-DEV] Accessors v2 Patch

Much better implementation.

As for the failed tests, I would double check and make sure you don't have any stray debug statements (e.g. printf).

Sent from my iPhone

https://bugs.php.net/patch-display.php?bug_id=49526&patch=accessor_v2.
diff&revision=latest

I rewrote part of how it handles the job due to abysmal performance of version 1. This one is near the same performance as without the code changes using __get() and is actually faster in some cases (public getter).

I also checked all the tests and fixed the issues I found related.

There is however some strange behavior that I do not understand. For some reason if I run all of the tests, 100+ or so will fail. If I have the test script write those failed tests to a file and just run those tests, all but four work fine. It appears to be something to do with line endings, in other cases the output does not match expected because of the addition of 'unicode' to the output. I've no idea what's going on here but after 3 days of mucking around, I can't find a solution as to why this is occurring.

These failures do not occur on a fresh checkout but nothing I've changed should be causing any of the issues, just no idea here, any help would be appreciated.

Felipe: I could not find any segmentation fault tests from before when you posted, nor now. Are you still seeing segmentation faults?

Thoughts?

-Clint

--
Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

13 years ago by Clint M Priest — view source — reply

unread

There are already two tests against private read and private write, should I add two for protected as well?

-----Original Message-----
From: Pierre Joye [mailto:pierre.php@gmail.com]
Sent: Sunday, December 11, 2011 6:36 PM
To: Clint M Priest
Cc: internals@lists.php.net
Subject: Re: [PHP-DEV] Accessors v2 Patch

Hi Clint,

Thanks for the updated patch :)

Some suggestions (did not test it yet only review it),

add test cases for non public property (private, protected)
show examples of the concept of readonly property (as it is allowed in the C# equivalent

I would also suggest to either take over the RFC (contact the original
author) or create your own to update it with a full list of examples, documentation, and all necessary information. This is a complex task with many possible (edge) cases. An updated RFC is a must to be able to discuss such additions.

Cheers,

https://bugs.php.net/patch-display.php?bug_id=49526&patch=v2.1&revisio
n=latest

Alright, nevermind on the failing tests, figured that out. I didn't realize the test structure doesn't indicate that it was segfaulting, that fixed all of the failing tests.

So... I guess this patch is up for discussion?

I don't presently have any static getters implemented, as best I can tell it would require some changes to opcodes, I didn't dive into anything besides exploration there.

-Clint

-----Original Message-----
From: Will Fitch [mailto:will.fitch@gmail.com]
Sent: Sunday, December 11, 2011 1:41 PM
To: Clint M Priest
Cc: internals@lists.php.net
Subject: Re: [PHP-DEV] Accessors v2 Patch

Much better implementation.

As for the failed tests, I would double check and make sure you don't have any stray debug statements (e.g. printf).

Sent from my iPhone

https://bugs.php.net/patch-display.php?bug_id=49526&patch=accessor_v2.
diff&revision=latest

I rewrote part of how it handles the job due to abysmal performance of version 1. This one is near the same performance as without the code changes using __get() and is actually faster in some cases (public getter).

I also checked all the tests and fixed the issues I found related.

There is however some strange behavior that I do not understand. For some reason if I run all of the tests, 100+ or so will fail. If I have the test script write those failed tests to a file and just run those tests, all but four work fine. It appears to be something to do with line endings, in other cases the output does not match expected because of the addition of 'unicode' to the output. I've no idea what's going on here but after 3 days of mucking around, I can't find a solution as to why this is occurring.

These failures do not occur on a fresh checkout but nothing I've changed should be causing any of the issues, just no idea here, any help would be appreciated.

Felipe: I could not find any segmentation fault tests from before when you posted, nor now. Are you still seeing segmentation faults?

Thoughts?

-Clint

--
Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

13 years ago by Pierre Joye — view source — reply

unread

oh right, I missed them. Yes, so it is covered as well :)

There are already two tests against private read and private write, should I add two for protected as well?

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

13 years ago by Clint M Priest — view source — reply

unread

To be complete I should probably add something to the reflection system as well. At present the getters/setters would show up as functions.

What would be preferable?

Show up as regular functions and let users fend for themselves?
Hide from getMethods() and:
2.1) Provide getAccessors() - Probably returning a new ReflectionPropertyAccessor class?
2.2) Provide getGetters(), getSetters()
Modify ReflectionProperty to include hasGetter() and hasSetter()

Comments?

-----Original Message-----
From: Pierre Joye [mailto:pierre.php@gmail.com]
Sent: Sunday, December 11, 2011 6:47 PM
To: Clint M Priest
Cc: internals@lists.php.net
Subject: Re: [PHP-DEV] Accessors v2 Patch

oh right, I missed them. Yes, so it is covered as well :)

There are already two tests against private read and private write, should I add two for protected as well?

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

13 years ago by guilhermeblanco@gmail.com — view source — reply

unread

I have just one question, partially unrelated.

How can I make something similar to Interceptors of Java according to
your approach?
For those that have no idea, interceptors is a way to intercept
get/set of a property inside the class and act under this
circumstance.

[]s,

To be complete I should probably add something to the reflection system as well. At present the getters/setters would show up as functions.

What would be preferable?

Show up as regular functions and let users fend for themselves?

Hide from getMethods() and:
2.1) Provide getAccessors() - Probably returning a new ReflectionPropertyAccessor class?
2.2) Provide getGetters(), getSetters()

Modify ReflectionProperty to include hasGetter() and hasSetter()

Comments?

-----Original Message-----
From: Pierre Joye [mailto:pierre.php@gmail.com]
Sent: Sunday, December 11, 2011 6:47 PM
To: Clint M Priest
Cc: internals@lists.php.net
Subject: Re: [PHP-DEV] Accessors v2 Patch

oh right, I missed them. Yes, so it is covered as well :)

There are already two tests against private read and private write, should I add two for protected as well?

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

--

--
Guilherme Blanco
MSN: guilhermeblanco@hotmail.com
GTalk: guilhermeblanco
Toronto - ON/Canada

13 years ago by Will Fitch — view source — reply

unread

This approach, in theory, is an interceptor itself. Most use-cases around this approach will be changing or augmenting an existing property that is not public. If you wanted to modify this interception, you'd need to extend the class using it and redefine the getter and/or setter.

I have just one question, partially unrelated.

How can I make something similar to Interceptors of Java according to
your approach?
For those that have no idea, interceptors is a way to intercept
get/set of a property inside the class and act under this
circumstance.

[]s,

To be complete I should probably add something to the reflection system as well. At present the getters/setters would show up as functions.

What would be preferable?

Show up as regular functions and let users fend for themselves?

Hide from getMethods() and:
2.1) Provide getAccessors() - Probably returning a new ReflectionPropertyAccessor class?
2.2) Provide getGetters(), getSetters()

Modify ReflectionProperty to include hasGetter() and hasSetter()

Comments?

-----Original Message-----
From: Pierre Joye [mailto:pierre.php@gmail.com]
Sent: Sunday, December 11, 2011 6:47 PM
To: Clint M Priest
Cc: internals@lists.php.net
Subject: Re: [PHP-DEV] Accessors v2 Patch

oh right, I missed them. Yes, so it is covered as well :)

There are already two tests against private read and private write, should I add two for protected as well?

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

--

--
Guilherme Blanco
MSN: guilhermeblanco@hotmail.com
GTalk: guilhermeblanco
Toronto - ON/Canada

13 years ago by guilhermeblanco@gmail.com — view source — reply

unread

hi Will,

That's what I've been thinking about.
I have a special necessity to overload a class in a Proxy
implementation (an instance that wraps a real Entity), demanding the
load when any getter is active.

This implementation seems to fit perfectly my needs, while still be cleaning.

I wonder if the user have already implemented a getter in this
approach, how can I overload this one?
Example:

class Foo
{
protected $name {
get { return $this->name; }
set { $this->name = $value; }
}
// ...
}

class FooProxy extends Foo
{
protected $name {
get { $this->__load(); parent::get(); }
set { $this->__load(); parent::set($value); }
}
// ...
}

How would it act in this situation?

Cheers,

This approach, in theory, is an interceptor itself. Most use-cases around this approach will be changing or augmenting an existing property that is not public. If you wanted to modify this interception, you'd need to extend the class using it and redefine the getter and/or setter.

I have just one question, partially unrelated.

How can I make something similar to Interceptors of Java according to
your approach?
For those that have no idea, interceptors is a way to intercept
get/set of a property inside the class and act under this
circumstance.

[]s,

To be complete I should probably add something to the reflection system as well. At present the getters/setters would show up as functions.

What would be preferable?

Show up as regular functions and let users fend for themselves?

Hide from getMethods() and:
2.1) Provide getAccessors() - Probably returning a new ReflectionPropertyAccessor class?
2.2) Provide getGetters(), getSetters()

Modify ReflectionProperty to include hasGetter() and hasSetter()

Comments?

-----Original Message-----
From: Pierre Joye [mailto:pierre.php@gmail.com]
Sent: Sunday, December 11, 2011 6:47 PM
To: Clint M Priest
Cc: internals@lists.php.net
Subject: Re: [PHP-DEV] Accessors v2 Patch

oh right, I missed them. Yes, so it is covered as well :)

There are already two tests against private read and private write, should I add two for protected as well?

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

--

--
Guilherme Blanco
MSN: guilhermeblanco@hotmail.com
GTalk: guilhermeblanco
Toronto - ON/Canada

--

--
Guilherme Blanco
MSN: guilhermeblanco@hotmail.com
GTalk: guilhermeblanco
Toronto - ON/Canada

13 years ago by Clint M Priest — view source — reply

unread

There is currently no parent::get(), parent::set() capability. The RFC has no mention of being able to call the parent accessor and so I have not explored that possibility. It would be a nice feature for just such a purpose.

-----Original Message-----
From: guilhermeblanco@gmail.com [mailto:guilhermeblanco@gmail.com]
Sent: Sunday, December 11, 2011 10:35 PM
To: Will Fitch
Cc: Clint M Priest; Pierre Joye; internals@lists.php.net
Subject: Re: [PHP-DEV] Accessors v2 Patch

hi Will,

That's what I've been thinking about.
I have a special necessity to overload a class in a Proxy implementation (an instance that wraps a real Entity), demanding the load when any getter is active.

This implementation seems to fit perfectly my needs, while still be cleaning.

I wonder if the user have already implemented a getter in this approach, how can I overload this one?
Example:

class Foo
{
protected $name {
get { return $this->name; }
set { $this->name = $value; }
}
// ...
}

class FooProxy extends Foo
{
protected $name {
get { $this->__load(); parent::get(); }
set { $this->__load(); parent::set($value); }
}
// ...
}

How would it act in this situation?

Cheers,

This approach, in theory, is an interceptor itself. Most use-cases around this approach will be changing or augmenting an existing property that is not public. If you wanted to modify this interception, you'd need to extend the class using it and redefine the getter and/or setter.

I have just one question, partially unrelated.

How can I make something similar to Interceptors of Java according to
your approach?
For those that have no idea, interceptors is a way to intercept
get/set of a property inside the class and act under this
circumstance.

[]s,

To be complete I should probably add something to the reflection system as well. At present the getters/setters would show up as functions.

What would be preferable?

Show up as regular functions and let users fend for themselves?

Hide from getMethods() and:
2.1) Provide getAccessors() - Probably returning a new ReflectionPropertyAccessor class?
2.2) Provide getGetters(), getSetters()

Modify ReflectionProperty to include hasGetter() and hasSetter()

Comments?

-----Original Message-----
From: Pierre Joye [mailto:pierre.php@gmail.com]
Sent: Sunday, December 11, 2011 6:47 PM
To: Clint M Priest
Cc: internals@lists.php.net
Subject: Re: [PHP-DEV] Accessors v2 Patch

oh right, I missed them. Yes, so it is covered as well :)

There are already two tests against private read and private write, should I add two for protected as well?

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

--
To unsubscribe,
visit: http://www.php.net/unsub.php

--
Guilherme Blanco
MSN: guilhermeblanco@hotmail.com
GTalk: guilhermeblanco
Toronto - ON/Canada

--
To unsubscribe,
visit: http://www.php.net/unsub.php

--
Guilherme Blanco
MSN: guilhermeblanco@hotmail.com
GTalk: guilhermeblanco
Toronto - ON/Canada

13 years ago by johannes@schlueters.de — view source — reply

unread

To be complete I should probably add something to the reflection system as well. At present the getters/setters would show up as functions.

What would be preferable?

Show up as regular functions and let users fend for themselves?

Hide from getMethods() and:
2.1) Provide getAccessors() - Probably returning a new ReflectionPropertyAccessor class?
2.2) Provide getGetters(), getSetters()

Modify ReflectionProperty to include hasGetter() and hasSetter()

Comments?

Reflection in PHP typically is leaking the implementation details
typically quite a lot instead of abstracting things away (interfaces and
Traits are ReflectionClass instances etc.)

I didn't look at the implementation but if these accessors are
implemented as methods (especially if it can be called just like one) I
would go for approach 1) and probably mark with a flag. For everything
else we'd probably have to clean-u reflection to leak less details to
make sense.

johannes

-----Original Message-----
From: Pierre Joye [mailto:pierre.php@gmail.com]
Sent: Sunday, December 11, 2011 6:47 PM
To: Clint M Priest
Cc: internals@lists.php.net
Subject: Re: [PHP-DEV] Accessors v2 Patch

oh right, I missed them. Yes, so it is covered as well :)

There are already two tests against private read and private write, should I add two for protected as well?

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

13 years ago by guilhermeblanco@gmail.com — view source — reply

unread

Hi,

I don't see how we could possibly do as option 1.
The structure would be weird, because considering I'm looking for an
specific ReflectionMethod that matches a property name, I'd also be
able to call it through invoke, but also regularly through
$obj->Setter($value);
Since I don't see in the match as possible to be executed as the
second possibility, I'd rather bring to a more centralized approach.

$reflProperty = $reflClass->getProperty('Hours');
$reflProperty->hasGetter() === true
$reflProperty->hasSetter() === true

Also,

$reflProperty->setValue($object, $value); would call the Setter method
as well as
$reflProperty->getValue($object) would call the Getter method

It seems the most consistent and centralized approach.

Regards,

2011/12/12 Johannes Schlüter johannes@schlueters.de:

To be complete I should probably add something to the reflection system as well. At present the getters/setters would show up as functions.

What would be preferable?

Show up as regular functions and let users fend for themselves?

Hide from getMethods() and:
2.1) Provide getAccessors() - Probably returning a new ReflectionPropertyAccessor class?
2.2) Provide getGetters(), getSetters()

Modify ReflectionProperty to include hasGetter() and hasSetter()

Comments?

Reflection in PHP typically is leaking the implementation details
typically quite a lot instead of abstracting things away (interfaces and
Traits are ReflectionClass instances etc.)

I didn't look at the implementation but if these accessors are
implemented as methods (especially if it can be called just like one) I
would go for approach 1) and probably mark with a flag. For everything
else we'd probably have to clean-u reflection to leak less details to
make sense.

johannes

-----Original Message-----
From: Pierre Joye [mailto:pierre.php@gmail.com]
Sent: Sunday, December 11, 2011 6:47 PM
To: Clint M Priest
Cc: internals@lists.php.net
Subject: Re: [PHP-DEV] Accessors v2 Patch

oh right, I missed them. Yes, so it is covered as well :)

There are already two tests against private read and private write, should I add two for protected as well?

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

--

--
Guilherme Blanco
MSN: guilhermeblanco@hotmail.com
GTalk: guilhermeblanco
Toronto - ON/Canada

13 years ago by Clint M Priest — view source — reply

unread

Why perpetuate the problem though? I'd tend to think 3 would be the best solution.

-----Original Message-----
From: Johannes Schlüter [mailto:johannes@schlueters.de]
Sent: Monday, December 12, 2011 6:52 PM
To: Clint M Priest
Cc: Pierre Joye; internals@lists.php.net
Subject: RE: [PHP-DEV] Accessors v2 Patch

To be complete I should probably add something to the reflection system as well. At present the getters/setters would show up as functions.

What would be preferable?

Show up as regular functions and let users fend for themselves?

Hide from getMethods() and:
2.1) Provide getAccessors() - Probably returning a new ReflectionPropertyAccessor class?
2.2) Provide getGetters(), getSetters()

Modify ReflectionProperty to include hasGetter() and hasSetter()

Comments?

Reflection in PHP typically is leaking the implementation details typically quite a lot instead of abstracting things away (interfaces and Traits are ReflectionClass instances etc.)

I didn't look at the implementation but if these accessors are implemented as methods (especially if it can be called just like one) I would go for approach 1) and probably mark with a flag. For everything else we'd probably have to clean-u reflection to leak less details to make sense.

johannes

-----Original Message-----
From: Pierre Joye [mailto:pierre.php@gmail.com]
Sent: Sunday, December 11, 2011 6:47 PM
To: Clint M Priest
Cc: internals@lists.php.net
Subject: Re: [PHP-DEV] Accessors v2 Patch

oh right, I missed them. Yes, so it is covered as well :)

There are already two tests against private read and private write, should I add two for protected as well?

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org