missing @trigger_error() call for this

@xabbuh done

It can now be:

instantiateComplexObject(array &$data, $class, $format, array &$context, \ReflectionClass $reflectionClass, $allowedAttributes)

To mimic the SerializerInterface::deserialize prototype.

my bad I kept the old signature, will change it

What do you think about using instantiateClass to have something shorter but still clear ?

Why removing the unset?

bad copy/paste, should be kept

ReflectionParameter::getType() should be used when available imo and exceptions catched in case the type hinted class doesn't exist.

I personally find it easier to check check if the class is null or not in this case

I agree but the type hint may be on a class not loadable. In this case, the exception thrown should be catched and the default value applied.

could you elaborate? On which scenario may it happen? Not that I'm not trusting you, but if that's the case I would like to add a test for it :)

In case you use a type hint with a class from a third party library which is not loaded because you don't need it for example.
And the advantage of getType() is that it doesn't need to load the class, so would work anyway.

Ah I see, fair enough will change that then

$format instead of null

Should not be updated imo as this is less clear...

The more indent you have the harder it is to read. And if the choice is to use if/else/elseif blocks, then everything should be in an else instead of the return done L311.

There is only one line here and i agree that most of the time else +return is only redundant but here it highlights the fact that one of this line will be executed and that the second one is not just a default.

Edit: BTW, we could reverse the condition if ($constructor) to have less lines indented as you said :)

Edit: BTW, we could reverse the condition if ($constructor) to have less lines indented as you said :)

I like this one :)

But not sure this would be accepted as this is a small detail and could create many merge conflicts.

Yeah actually I think I'll revert everything for that part, it's just CS and although I think it makes a big difference it terms of readability, I fear that it will cause lots of conflicts later on. One solution would be do go back in earlier versions and fix that version after version, but it's rather tedious and slow.

Unfortunatelly this looks like a bc break as people may depend on the call to instantiateObject.

Not sure to see what you mean, before it was simply failing so people could not used it. The method has been changed because there's a BC break in the method signature (we are adding $format), but the behaviour is the same as the previous one.

In case someone extends instantiateObject it won't be called anymore. I'll provide you an example of how this could be managed.

You could do something like:

protected function instantiateObject(..., $allowedAttributes/**, $format*/)
{
    $format = null;
    if (func_num_args() >= 6) {
        $format = func_get_arg(5);
    }

    // ...
}

protected function instantiateComplexObject(...)
{
    static $deprecationTriggered = false;
    if (false === $deprecationTriggered) {
        $deprecationTriggered = true;
        $class = get_class($this);
        if (0 !== strpos($class, 'Symfony\\') 
            && self::class !== (new \ReflectionMethod($this, 'instantiateObject'))->getDeclaringClass()->getName()) {
            @trigger_error(..., E_USER_DEPRECATED);
        }
    }

    return $this->instantiateObject(...);
}

taking $format as the 6th argument in instantiateObject would still be a problem if you extended it

This is forbidden by the PHP interpreter itself: https://3v4l.org/S48gf
Si there is no problem here.

When you override a protected method, you must respect its contract... It's not a BC break for me.

If you override it this way there is not BC as the added param is optional.

In all honesty I would be happy to say yes to this solution it's the easiest way to deal with it really. The only thing I'm worried about is that it may introduce a BC break (but that's up to you to decide if it is or not) and the fact that the user may not see the change (i.e. see that he has to inject the format now).

Anyway, it's not a BC break because the parent function is still called with the good number of arguments. See https://3v4l.org/XpYGK.

Nvm, even with my example it wouldn't be a problem as you would call the parent function with the right number of parameters. Not doing so is looking for trouble and would be the developer fault.

This solution is often used in symfony so i'd say it's not part of the symfony bc promises.