Skip to content

[PropertyAccess] consistency in createPropertyAccessor #19040

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

[PropertyAccess] consistency in createPropertyAccessor #19040

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 13 additions & 20 deletions src/Symfony/Component/PropertyAccess/PropertyAccess.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@

namespace Symfony\Component\PropertyAccess;

use Psr\Cache\CacheItemPoolInterface;

/**
* Entry point of the PropertyAccess component.
*
Expand All @@ -19,39 +21,30 @@
final class PropertyAccess
{
/**
* Creates a property accessor with the default configuration.
* Creates a property accessor.
*
* For dealing with several property accessor configured differently, use
* the createPropertyAccessorBuilder() method instead.
*
* @param bool $throwExceptionOnInvalidIndex
* @param bool $magicCall
* @param bool $throwExceptionOnInvalidIndex
* @param bool $magicCall
* @param CacheItemPoolInterface|null $cacheItemPool
*
* @return PropertyAccessor The new property accessor
*/
public static function createPropertyAccessor($throwExceptionOnInvalidIndex = false, $magicCall = false)
public static function createPropertyAccessor($magicCall = false, $throwExceptionOnInvalidIndex = false, CacheItemPoolInterface $cacheItemPool = null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those properties should not exist as well because that is where the builder should be used. otherwise this static method is just a wrapper around the constructor of PropertyAccessor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order of $magicCall and $throwExceptionOnInvalidIndex shouldn't be inverted (it's a BC break).

Copy link
Member Author

@chalasr chalasr Jun 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise this static method is just a wrapper around the constructor of PropertyAccessor.

It was already before, just more restrictive due to the missing arguments.

Also I understand your point of view about the usefulness of this, but most of time there is no need of the builder itself because these arguments are defined once only when getting the PropertyAccessor instance. IMO the builder should be used to deal with differently configured property accessor, enabling features via the builder methods rather than re-pass all the PropertyAccessor constructor args each time you want to change one of its properties..

Plus I don't see the value of creating a PropertyAccessorBuilder instance + adding an additional method call (getPropertyAccessor()) that does exactly the same logic that new PropertyAccessor(...) does.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So is this change a BC break? (inversed args)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chalasr : No it isn't, as you introduced them in https://github.com/symfony/symfony/pull/18977/files#diff-223e0a8a8125ad444ff1668096eeb7deR28, which targets the same branch in development.

I guess @dunglas was simply not aware of that, which mislead him.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ogizanagi exactly.

{
return self::createPropertyAccessorBuilder($throwExceptionOnInvalidIndex, $magicCall)->getPropertyAccessor();
return new PropertyAccessor($magicCall, $throwExceptionOnInvalidIndex, $cacheItemPool);
}

/**
* Creates a property accessor builder.
*
* @param bool $enableExceptionOnInvalidIndex
* @param bool $enableMagicCall
*
* @return PropertyAccessorBuilder The new property accessor builder
*/
public static function createPropertyAccessorBuilder($enableExceptionOnInvalidIndex = false, $enableMagicCall = false)
public static function createPropertyAccessorBuilder()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why those changes? It's a BC break too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These args have been introduced in #18977 that has been merged the last week, the changes are present only in the master branch (3.2-dev). Is it even considered as a BC break to fix a mistake?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change is fine from the BC perspective.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it.

{
$propertyAccessorBuilder = new PropertyAccessorBuilder();

if ($enableExceptionOnInvalidIndex) {
$propertyAccessorBuilder->enableExceptionOnInvalidIndex();
}

if ($enableMagicCall) {
$propertyAccessorBuilder->enableMagicCall();
}

return $propertyAccessorBuilder;
return new PropertyAccessorBuilder();
}

/**
Expand Down
14 changes: 7 additions & 7 deletions src/Symfony/Component/PropertyAccess/Tests/PropertyAccessTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,24 +13,24 @@

use Symfony\Component\PropertyAccess\PropertyAccess;
use Symfony\Component\PropertyAccess\PropertyAccessor;
use Psr\Cache\CacheItemPoolInterface;

/**
* @author Robin Chalas <robin.chalas@gmail.com
* @author Robin Chalas <robin.chalas@gmail.com>
*/
final class PropertyAccessTest extends \PHPUnit_Framework_TestCase
{
public function testCreatePropertyAccessor()
{
$this->assertInstanceOf(PropertyAccessor::class, PropertyAccess::createPropertyAccessor());
}

public function testCreatePropertyAccessorWithExceptionOnInvalidIndex()
{
// magicCall enabling
$this->assertInstanceOf(PropertyAccessor::class, PropertyAccess::createPropertyAccessor(true));
}

public function testCreatePropertyAccessorWithMagicCallEnabled()
{
// throwExceptionOnInvalidIndex enabling
$this->assertInstanceOf(PropertyAccessor::class, PropertyAccess::createPropertyAccessor(false, true));

// cacheItemPool enabling
$this->assertInstanceOf(PropertyAccessor::class, PropertyAccess::createPropertyAccessor(false, false, $this->getMock(CacheItemPoolInterface::class)));
}
}