Better support for internal PHP classes default parameters

[Ocramius]

As @guilhermeblanco suggested, we can fully support internal PHP classes methods' default parameter values by using a trick like following.

Assuming this is an internal php class, and that we cannot discover the default value for $foo via reflection:

class SomeInternalClass
{
    public function doStuff($foo = 'bar') {}
}

This code should become like following in the proxy:

class MyProxy extends SomeInternalClass
{
    public function doStuff($foo = ProxyManager::PLACEHOLDER)
    {
        if (ProxyManager::PLACEHOLDER === $foo) {
            parent::doStuff(); // ignore parameter, use defaults
        } else {
            parent::doStuff($foo);
        }
    }
}

That will allow us to fully support default values in proxies for internal PHP classes.

Please note that this does not apply to class-hinted parameters, as the default value for a class-hinted parameter can only be null

[Ocramius]

Ping @mnapoli you were interested in this.

[mnapoli]

Thanks. What is the value of ProxyManager::PLACEHOLDER? Don't you risk some conflicts if you set it to an arbitrary value?

[staabm]

In Java you would use something like PLACEHOLDER = new Object() and do a strict comparison, to make it only match itself.

[mnapoli]

OK if it's a specific object instance then that's fine, I was afraid it was going to be something like null, or something more creative to avoid collision in most cases like NaN or PI or infinity :p

[mnapoli]

(using Nan or PI or infinity would be stupid right… it's not like anyone did it at some point in their life right… at least I didn't… I mean, maybe I just thought about using it… it was just one time… :p)

[lisachenko]

@mnapoli +1, I also thought about NaN ) but it was just one time too )) I like very much this crazy check:

$value = ...
if ($value !== $value) { // happy debug
    echo "It's crazy NaN value";
}

[Ocramius]

@staabm sadly no java :(

@mnapoli we can randomize/generate the value to ensure that it is always unique

[mnapoli]

@mnapoli we can randomize/generate the value to ensure that it is always unique

Risky :p

[staabm]

how about a float with a lot of number behind the comma, so it gets very unlikely it can match any existing real world numbers..?

[Ocramius]

It will just be a string ;-)

[Ocramius]

Deferred to 2.0.0: too much of an edge case, and = null is pretty much always the default.

[Ocramius]

Removed from 2.0.0 milestone: while this can be implemented, I'd rather have something like https://github.com/Roave/BetterReflection deal with this sort of problem instead.

[Ocramius]

So please, take it more seriously and at least put a disclaimer.

Sorry, but unless you have practical advice on how to resolve the issue, this is not constructive feedback.

Patches welcome, but I can't do much about internal symbols reflection being incomplete, because that's just how the engine behaves for types not defined in userland.

[Ocramius]

For your specific Redis issues, check the related issues here:

• [ProxyManagerBridge] Invalid code generated for \Redis lazy service symfony/symfony#23066
• Not working with the Redis extension BedrockStreaming/RedisMock#46
• Invalid Redis Proxy #461
• 2.1.5 (lazy loading) somehow breaks set method on session client snc/SncRedisBundle#442

Specifically, from that last issue:

Btw, this is neither a bug in ProxyManager nor in this bundle: you will need to either fix this in the extension, or provide a userland shim that can be reflected.

[alekitto]

Trying to be constructive through a proposal: what about loading class definitions for these edge-cases from a stub file? Could it be viable?

[Ocramius]

Putting a disclaimer that library have edge-cases in README is a constructive feedback

Please re-read your comment, then, and perhaps ask someone to analyze it for you, so you can understand why it is not an acceptable human interaction:

• with strangers you don't know personally
• that provide you with a free service
• that put lots of effort in it
• that operate very seriously, with a long and regular track record
• that, even after your inconstructive feedback, took the time to provide you with a set of references to similar problems, including potential workarounds

[Ocramius]

Trying to be constructive through a proposal: what about loading class definitions for these edge-cases from a stub file? Could it be viable?

@alekitto yes, see:

• Better support for internal PHP classes default parameters #162 (comment)
• Move internal implementation from Zend\Code to PHP-Parser #49

Beware that any mis-alignment between stubs and actual runtime symbols can still lead to problems.

Using something like BetterReflection is viable, but requires major rework of internals.

[azjezz]

@mnapoli we can randomize/generate the value to ensure that it is always unique

I think the best approach would be to use a combination of time, current directory/filename, and a pseudo random id.

$placeholder = uniqid(true, '__proxy_manager') . time() . hash('md5', __FILE__);

md5 because performance

[nicolas-grekas]

There's only one sensible way fix to the issue with Redis or similar: send a patch to https://github.com/phpredis/phpredis/ It's an actively maintained lib. Asking OSS authors to write and maintain piles of code that are just workarounds is a waste of their time. Even if one doesn't know C, one can patch the definitions in the source code by using copy/paste and imitation. Please consider sending them a PR @ilya-levin-lokalise!

As far as this issue is concerned, what about closing it @Ocramius, for the reason I just gave? The PHP engine is getting stricter and stricter regarding type declarations, it's not like extension authors have any excuses.

[cmb69]

It seems that issue has already been resolved. Proper reflection should be available as of 5.3.5RC1 in combination with PHP 8.

[Ocramius]

As far as this issue is concerned, what about closing it @Ocramius, for the reason I just gave? The PHP engine is getting stricter and stricter regarding type declarations, it's not like extension authors have any excuses.

Depends on whether this can still occur in the engine at all 🤔

[nicolas-grekas]

Depends on whether this can still occur in the engine at all thinking

I think yes, having reflection out of sync from actual declaration is still possible, but it's now way easier to have them in sync AFAIK.
Maybe @nikic has some more info on the topic?

[cmb69]

Depends on whether this can still occur in the engine at all 🤔

Yes, it still can happen. In some cases, a default cannot be specified (in this case because the parameter has no default; in other cases because the function may be overloaded like array_keys), and of course in some cases, because an extension has not yet been updated. Wrt. to the core and bundled extensions, there are only "few" such cases.

[Ocramius]

Yeah, problem is potentially still with ext-* stuff, including proprietary software. For proprietary stuff, there is a hard stop in such cases, because there would be no stubs either.

[nicolas-grekas]

Idea (likely a bad one): use a variadic argument in place of this UNKNOWN default?

[azjezz]

actually that's a pretty good one, using

function foo($a = null, /* signature becomes broken here, use variadic argument */ ...$rest)

for

function foo($a = null, $b = UNKNOWN, $c = null)

could work, people get auto-completion based on the original class, so the signature of the proxy doesn't matter.

https://3v4l.org/u0V0a

[Ocramius]

Overall potentially feasible, need to build some trickery for point-cut proxies, where the parameters are passed to interceptor callables (and keys may be missing), but it is likely feasible 🤔

[nicolas-grekas]

Another idea: set null as default value and forward the call with parent::doStuff(...func_get_args()).