Convert resources to objects in ext/ldap #6770
Conversation
kocsismate
force-pushed
the
ldap-resource
branch
5 times, most recently
from
c913aa4
to
847333d
Compare
kocsismate
force-pushed
the
ldap-resource
branch
6 times, most recently
from
9c491fd
to
fbbec09
Compare
kocsismate
force-pushed
the
ldap-resource
branch
from
fbbec09
to
d124f0e
Compare
|
@MCMic your review is welcome :) |
| @@ -2,265 +2,151 @@ | |||
|
|
|||
| /** @generate-class-entries */ | |||
|
|
|||
| /** @strict-properties */ | |||
| final class LDAP | |||
There was a problem hiding this comment.
Just to throw it out there, maybe make it LDAPConnection?
There was a problem hiding this comment.
Yeah, that's also a possibility. I chose LDAP because I thought it's more consistent with other extensions: e.g. with mysqli, PDO classes. But I've just found out that we now have FTPConnection, and we also chose CurlHandle.. So I'm not sure what to do here. But I'm open to change it to LDAPConnection.
kocsismate
force-pushed
the
ldap-resource
branch
from
e541412
to
db46da3
Compare
kocsismate
force-pushed
the
ldap-resource
branch
from
db46da3
to
fbdf507
Compare
|
@MCMic I've just applied these changes, but I can do some amendments if you find anything that should be changed. |
|
Hello @nikic @kocsismate sorry for the delay. Thank you for the work on php-ldap. Has the ship sailed for using a common namespace for the 3 classes? I’m fine with the name LDAP given the parameter name is always $ldap it’s consistent. (if we kept $link it would’ve been LDAPLink) Is it expected that the LDAP object has no properties and would it make sense to add some (like the parameters passed to ldap_connect) ? Am I right to consider that this is a step towards adding an object oriented interface or is that not a goal? If we do so, would it be acceptable to add methods which are not direct aliases on existing functions, to fix inconsistencies, and for example removing the _ext suffix to have only LDAP::bind working like ldap_bind_ext($ldap)? |
I think the ship has not even started, since doing so would need https://wiki.php.net/rfc/namespaces_in_bundled_extensions first.
My main goal is to get rid of as many resource type hints as possible, but yes, the resource to object migration initiative serves as a great potential to add better OO interfaces. Personally, I would be very happy if we moved towards OO APIs. So it makes sense for me to add properties as well as new methods to the newly introduced LDAP classes.
Yes, I believe so. |
…chalasr) This PR was merged into the 4.4 branch. Discussion ---------- [Ldap] Fix `resource` type checks & docblocks on PHP 8.1 | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - From https://www.php.net/manual/en/migration81.incompatible.php: > Several resources have been migrated to objects. Return value checks using is_resource() should be replaced with checks for false. The LDAP functions now accept and return, respectively, LDAP\ResultEntry objects instead of ldap result entry resources. The LDAP functions now accept and return, respectively, LDAP\Connection objects instead of ldap link resources. The LDAP functions now accept and return, respectively, LDAP\Result objects instead of ldap result resources. The LDAP functions now accept and return, respectively, LDAP\ResultEntry objects instead of ldap result entry resources. See also php/php-src#6770 Commits ------- 2c0f7e0 [LDAP] Fix resource type checks & docblocks on PHP 8.1
No description provided.