Skip to content

[PropertyAccess] Allow custom methods on property accesses #18016

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 18 commits into from
Closed

[PropertyAccess] Allow custom methods on property accesses #18016

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
af3a824
[PropertyAccess] Allow custom methods on property accesses
lrlopez Mar 4, 2016
5b82237
Added support for YAML and XML mapping
lrlopez Mar 6, 2016
00a26eb
Renamed Attribute to Property on most classes and methods
lrlopez Mar 16, 2016
e4aaac9
Annotation renamed to @Property
lrlopez Mar 17, 2016
e46df98
Added method annotation for virtual properties
lrlopez Mar 21, 2016
5f206e6
Renamed "attribute" to "property" in XSD and YAML
lrlopez Mar 21, 2016
94e6157
Fixed the metadata caching mess
lrlopez Mar 21, 2016
09707ad
Replaced metadata caching with standard PSR-6 cache pool
lrlopez Mar 22, 2016
d02d93a
Added support to metadata factories in PropertyAccesorBuilder
lrlopez Mar 25, 2016
748b894
Fix outdated type hint in LazyLoadingMetadataFactory
lrlopez Mar 29, 2016
688cbda
Fixed typo in variable name
lrlopez Apr 1, 2016
003efdb
Added 'Property' prefix to method annotations
lrlopez Apr 2, 2016
dc5cacb
Renamed 'propertiesMetadata' to 'propertyMetadataCollection' in 'Clas…
lrlopez Apr 2, 2016
1a35d45
Minor rebase fixes
lrlopez Jul 3, 2016
1e29523
Addressed @xabbuh and @fabpot comments
lrlopez Jul 3, 2016
7c6a79c
Make fabbot.io happy :)
lrlopez Jul 3, 2016
d0bd777
Fixed class name clash in FrameworkExtension
lrlopez Jul 3, 2016
6b7eeff
Revert unintended change in composer.json
lrlopez Jul 4, 2016
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
Prev Previous commit
Next Next commit
Annotation renamed to @Property
  • Loading branch information
@lrlopez
lrlopez committed Jul 3, 2016
commit e4aaac903a7bea8b30b83c509d3ddca76b9a4d9c
18 changes: 9 additions & 9 deletions ...rtyAccess/Annotation/PropertyAccessor.php → ...nt/PropertyAccess/Annotation/Property.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,33 +19,33 @@
*
* @author Luis Ramón López <lrlopez@gmail.com>
*/
class PropertyAccessor
class Property
Copy link
Contributor

Choose a reason for hiding this comment

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

This annotation duplicates the other annotations. I don't think we should add multiple ways to add the same thing:

  • more code to maintain
  • harder to improve performance
  • harder to introduce changes consistently
  • harder to understand for our users when to use which

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are also some advantages of using property annotations:

  • the annotation is next to the property declaration, which is very convenient when looking for it. On the other hand, method annotations are spread out in the class definition (properties are usually placed on the top of the class and method implementations at the end)
  • you can override several methods with just one annotation

Property annotations are implemented with just 19 lines of code (not taking into account the empty annotation class). In fact, all new annotations are loaded from the same method AnnotationLoader->loadClassMetadata(), so the code will be easy to maintain and optimize as changes would be centralized.

About being harder to understand for newcomers, we could just give them some initial guidelines (i.e. method annotations for virtual properties and property annotations for everything else) and let them use whatever they want, just like the YAML vs XML vs Annotation debate.

Anyway, if you feel that only method annotations must be kept, I'll do that, of course!

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @webmozart, it's weird to have different way to do the same thing. What about having only a @Property annotation with an optional name attribute in case you use it on a method?

Copy link
Contributor

Choose a reason for hiding this comment

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

@dunglas @webmozart 👍 for having only one annotation. @Property looks weird on a method, maybe @Accessor ?

{
/**
* Custom setter method for the property
* Custom setter method for the property.
*
* @var string $setter
* @var string
*/
public $setter;

/**
* Custom getter method for the property
* Custom getter method for the property.
*
* @var string $setter
* @var string
*/
public $getter;

/**
* Custom adder method for the property
* Custom adder method for the property.
*
* @var string $setter
* @var string
*/
public $adder;

/**
* Custom remover method for the property
* Custom remover method for the property.
*
* @var string $setter
* @var string
*/
public $remover;
}
4 changes: 2 additions & 2 deletions src/Symfony/Component/PropertyAccess/Mapping/Loader/AnnotationLoader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
namespace Symfony\Component\PropertyAccess\Mapping\Loader;

use Doctrine\Common\Annotations\Reader;
use Symfony\Component\PropertyAccess\Annotation\PropertyAccessor;
use Symfony\Component\PropertyAccess\Annotation\Property;
use Symfony\Component\PropertyAccess\Mapping\PropertyMetadata;
use Symfony\Component\PropertyAccess\Mapping\ClassMetadata;

Expand Down Expand Up @@ -56,7 +56,7 @@ public function loadClassMetadata(ClassMetadata $classMetadata)

if ($property->getDeclaringClass()->name === $className) {
foreach ($this->reader->getPropertyAnnotations($property) as $annotation) {
if ($annotation instanceof PropertyAccessor) {
if ($annotation instanceof Property) {
$propertiesMetadata[$property->name]->setGetter($annotation->getter);
$propertiesMetadata[$property->name]->setSetter($annotation->setter);
$propertiesMetadata[$property->name]->setAdder($annotation->adder);
Expand Down
6 changes: 3 additions & 3 deletions src/Symfony/Component/PropertyAccess/Tests/Fixtures/Dummy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,20 @@

namespace Symfony\Component\PropertyAccess\Tests\Fixtures;

use Symfony\Component\PropertyAccess\Annotation\PropertyAccessor;
use Symfony\Component\PropertyAccess\Annotation\Property;

/**
* Fixtures for testing metadata.
*/
class Dummy
{
/**
* @PropertyAccessor(getter="getter1", setter="setter1", adder="adder1", remover="remover1")
* @Property(getter="getter1", setter="setter1", adder="adder1", remover="remover1")
*/
protected $foo;

/**
* @PropertyAccessor(getter="getter2")
* @Property(getter="getter2")
*/
protected $bar;

Expand Down
6 changes: 2 additions & 4 deletions src/Symfony/Component/PropertyAccess/Tests/Fixtures/TestClass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,12 @@

namespace Symfony\Component\PropertyAccess\Tests\Fixtures;

use Doctrine\ORM\Mapping\Column;
use Symfony\Component\PropertyAccess\Annotation\PropertyAccessor;
use Symfony\Component\PropertyAccess\Annotation\Property;

class TestClass
{
public $publicProperty;
protected $protectedProperty;
private $privateProperty;

private $publicAccessor;
private $publicMethodAccessor;
Expand All @@ -32,7 +30,7 @@ class TestClass
private $date;

/**
* @PropertyAccessor(getter="customGetterTest", setter="customSetterTest")
* @Property(getter="customGetterTest", setter="customSetterTest")
*/
private $customGetterSetter;

Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/PropertyAccess/Tests/PropertyAccessorCollectionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class PropertyAccessorCollectionTest_Car
private $axes;

/**
* @Symfony\Component\PropertyAccess\Annotation\PropertyAccessor(adder="addAxisTest", remover="removeAxisTest")
* @Symfony\Component\PropertyAccess\Annotation\Property(adder="addAxisTest", remover="removeAxisTest")
*/
private $customAxes;

Expand Down