This can respect bc if you use func_num_args and func_get_arg. (as in this example)

Many thanks @Ener-Getick, I was not sure about if it could be envisaged.
If this is merged, should I reference somewhere that the argument should be really added to the method signature in 4.0?

EDIT
I just saw that you also modified the interface by adding a commented param.

LGTM 👍

Added doc PR and new tests.

Why do we care about adding an optional argument to the PropertyAccess::createPropertyAccessor method ? The class is final and thus cannot be extended, nor is part of an interface, right ? It appears there is no BC break to me.

Indeed @ogizanagi i didn't realize that this class is final. So we don't need any bc layer.

Me too, will remove this hack. Thank's @ogizanagi

👍

What about adding the $magicCall argument, too ?

Done @ogizanagi

Thank you @chalasr.

@fabpot Out of curiosity, why did you create a merge commit that closed this rather than doing "Squash and merge" from here directly?

He is not doing it via the web interface. The symfony guys are using a proprietary CLI tool 😄

And we hope it will be open sourced sometime 👍

Unfortunately, best tools are (too) often hidden... ^^ Thank's for the explanation @OskarStark

Unfortunately, best tools are (too) often hidden... ^^

you are right 😃

I'm 👎 on this PR. It doesn't make sense and the use case it covers is already possible by using the builder. Also it does't expose all arguments as the cache argument is not exposed which makes it even inconsistent.

@Tobion the PropertyAccessBuilder::cacheItemPool is not available in 2.7.

IMHO the problem is that having two ways of getting a PropertyAccessor instance is a bit confusing.
We should be able to create a PropertyAccessor instance with all its arguments without need of switching between PropertyAccess::createPropertyAccessor() and PropertyAccessorBuilder::getPropertyAccessor().

What about making these two classes only one?

One is a builder and the other the actual accessor. They are meant to be used differently. This is exactly why this PR makes no sense.

the PropertyAccessBuilder::cacheItemPool is not available in 2.7.

What does this have to do with this PR?

One is a builder and the other the actual accessor. They are meant to be used differently.

One is the builder, but the other is not the accessor (PropertyAccess class), it just creates an instance of the builder that returns the accessor. This is the reason of this PR, the documented and most used way is to use PropertyAccess::createPropertyAccessor.
I see the PropertyAccess class as the right way to get a PropertyAccessor instance, as said in the PropertyAccessor itself:

/**
 * Should not be used by application code. Use
 * {@link PropertyAccess::createPropertyAccessor()} instead.
 *
 * @param bool $magicCall
 * @param bool $throwExceptionOnInvalidIndex
 */
public function __construct($magicCall = false, $throwExceptionOnInvalidIndex = false)

What is the point of having a PropertyAccess::createPropertyAccessor() if it can't use the PropertyAccessor arguments? We have two ways to get the same result, except that the first doesn't take benefit of the accessor constructor arguments and the second does.
I don't see the value of having the first before this PR, otherwise let's just use the builder directly and document it as the way to go.

I agree with @Tobion.

Reading the other PR adding the cache pool feels wrong.

My understanding is that the createPropertyAccessor() method creates a property accessor with good defaults. And if you need to tweak the config, you need to use the builder. Also, using the builder is better as instead of passing "random" booleans as argument, you call explicit method names. And when adding some more configuration, supporting it is just a matter of adding a method to the builder, no need to change the arguments of the PropertyAccess class.

So, I'm going to revert this PR instead of merging #19040.

reverted

In fact, this was firstly motivated by the default configuration provided in the PropertyAccess::createPropertyAccessor() method, disabling exceptions on invalid indexes and so making PropertyAccessor::isReadable() always returning true, even for undefined indexes.

IMHO the throwExceptionOnInvalidIndex argument should only make the getValue() method returning null in case of invalid indices if set to false and throw an exception if set to true, but it should never change the return of the isReadable() method because it makes it wrong.
See #18969 for an example.

Having faced this issue made me thinking too restrictively, I totally understand the reverting of this.