[PropertyInfo] Import the component #15858

dunglas · 2015-09-21T15:03:40Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	n/a
License	MIT
Doc PR	symfony/symfony-docs#5717

As discussed with @fabpot (see #14844), this PR moves dunglas/php-property-info under the Symfony umbrella.

Rationale behind this new component (extracted from README.md):

PHP doesn't support explicit type definition. This is annoying, especially when doing meta programming.
Various libraries including but not limited to Doctrine ORM and the Symfony Validator provide their own type managing
system.
This library extracts various information including the type and documentation from PHP class property from metadata of popular sources:

Setter method with type hint
PHPDoc DocBlock
Doctrine ORM mapping (annotation, XML, YML or custom format)
PHP 7 scalar typehint and return type
Serializer metadata

Usage:

<?php

// Use Composer autoload
require 'vendor/autoload.php';

use Doctrine\ORM\EntityManager;
use Doctrine\ORM\Tools\Setup;
use Doctrine\ORM\Mapping\Column;
use Doctrine\ORM\Mapping\Entity;
use Doctrine\ORM\Mapping\Id;
use Symfony\Component\PropertyInfo\Extractors\DoctrineExtractor;
use Symfony\Component\PropertyInfo\Extractors\PhpDocExtractor;
use Symfony\Component\PropertyInfo\Extractors\ReflectionExtractor;
use Symfony\Component\PropertyInfo\PropertyInfo;

/**
 * @Entity
 */
class MyTestClass
{
    /**
     * @Id
     * @Column(type="integer")
     */
    public $id;
    /**
     * This is a date (short description).
     *
     * With a long description.
     *
     * @var \DateTime
     */
    public $foo;
    private $bar;

    public function setBar(\SplFileInfo $bar)
    {
        $this->bar = $bar;
    }
}

// Doctrine initialization (necessary only to use the Doctrine Extractor)
$config = Setup::createAnnotationMetadataConfiguration([__DIR__], true);
$entityManager = EntityManager::create([
    'driver' => 'pdo_sqlite',
    // ...
], $config);

$doctrineExtractor = new DoctrineExtractor($entityManager->getMetadataFactory());
$phpDocExtractor = new PhpDocExtractor();
$reflectionExtractor = new ReflectionExtractor();

$propertyInfo = new PropertyInfo(
    array($reflectionExtractor),
    array($doctrineExtractor, $phpDocExtractor, $reflectionExtractor),
    array($phpDocExtractor),
    array($reflectionExtractor)
);

var_dump($propertyInfo->getProperties('MyTestClass'));
var_dump($propertyInfo->getTypes('MyTestClass', 'foo'));
var_dump($propertyInfo->getTypes('MyTestClass', 'id'));
var_dump($propertyInfo->getTypes('MyTestClass', 'bar'));
var_dump($propertyInfo->isReadable('MyTestClass', 'id'));
var_dump($propertyInfo->isReadable('MyTestClass', 'bar'));
var_dump($propertyInfo->isWritable('MyTestClass', 'foo'));
var_dump($propertyInfo->isWritable('MyTestClass', 'bar'));
var_dump($propertyInfo->getShortDescription('MyTestClass', 'foo'));
var_dump($propertyInfo->getLongDescription('MyTestClass', 'foo'));

Output:

array(3) {
  [0] =>
  string(2) "id"
  [1] =>
  string(3) "foo"
  [2] =>
  string(3) "Bar"
}
array(1) {
  [0] =>
  class Symfony\Component\PropertyInfo\Type#36 (6) {
    private $builtinType =>
    string(6) "object"
    private $nullable =>
    bool(false)
    private $class =>
    string(8) "DateTime"
    private $collection =>
    bool(false)
    private $collectionKeyType =>
    NULL
    private $collectionValueType =>
    NULL
  }
}
array(1) {
  [0] =>
  class Symfony\Component\PropertyInfo\Type#36 (6) {
    private $builtinType =>
    string(3) "int"
    private $nullable =>
    bool(false)
    private $class =>
    NULL
    private $collection =>
    bool(false)
    private $collectionKeyType =>
    NULL
    private $collectionValueType =>
    NULL
  }
}
array(1) {
  [0] =>
  class Symfony\Component\PropertyInfo\Type#245 (6) {
    private $builtinType =>
    string(6) "object"
    private $nullable =>
    bool(false)
    private $class =>
    string(11) "SplFileInfo"
    private $collection =>
    bool(false)
    private $collectionKeyType =>
    NULL
    private $collectionValueType =>
    NULL
  }
}
bool(true)
bool(false)
bool(true)
bool(true)
string(35) "This is a date (short description)."
string(24) "With a long description."

dunglas · 2015-09-21T15:13:19Z

src/Symfony/Component/PropertyInfo/Extractors/DoctrineExtractor.php

+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ * (c) Kévin Dunglas <dunglas@gmail.com>


Not sure if it is required or not as this code was in the wild previously but I'm ok to remove this line if necessary.

I'm no license expert, but I think it would be better to be consistent with everything. We can talk about that on the phone.

mickaelandrieu · 2015-09-21T15:26:10Z

👍 this is realy useful

stof · 2015-09-21T15:34:35Z

src/Symfony/Component/PropertyInfo/Extractors/DoctrineExtractor.php

+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\PropertyInfo\Extractors;


we generally use singular for namespaces in Symfony

theofidry · 2015-09-21T18:30:26Z

👍

dbu · 2015-09-21T18:52:53Z

what about the doctrine variants (mongo, phpcr)? can they provide their own extractor in their symfony bundle?

stof · 2015-09-21T19:02:07Z

src/Symfony/Bridge/Doctrine/PropertyInfo/DoctrineExtractor.php

+                    return array(new Type(Type::BUILTIN_TYPE_OBJECT, $nullable, 'DateTime'));
+
+                case 'array':
+                    return array(new Type(Type::BUILTIN_TYPE_ARRAY, $nullable, null, true, new Type(Type::BUILTIN_TYPE_INT)));


this supports string keys too

dunglas · 2015-09-21T19:40:19Z

@dbu IMO it could even be added directly in the Doctrine bridge.

fabpot · 2015-09-22T11:19:11Z

src/Symfony/Component/PropertyInfo/README.md

+    composer require symfony/property-info
+
+To use the PHPDoc extractor, install the [phpDocumentator's Reflection](https://github.com/phpDocumentor/Reflection) library.
+To use the Doctrine extractor, install the [Doctrine ORM](http://www.doctrine-project.org/projects/orm.html).


You should mention that you also need the Doctrine bridge.

fabpot · 2015-09-22T11:23:12Z

Can you rebase to get rid of all the merge commits that we don't want to keep when merging?

fabpot · 2015-09-22T11:23:25Z

LGTM

stof · 2015-09-22T11:28:47Z

before the merge here, I would like to see the documentation PR being opened, which will help us see how this component is mean to be used (as we will be able to read the documentation)

dunglas · 2015-09-22T16:09:43Z

@stof doc added
@fabpot CS and license fixed. Rebasing is a pain because of all files in common at the root in old versions. If merge commits are really a problem, IMO the best is to get rid of the history. But maybe some Git gurus here know a solution to avoid all those rebasing conflicts.

dunglas · 2015-09-22T19:47:25Z

The AppVeyor error is unrelated.

fabpot · 2015-09-23T09:32:12Z

@fabpot I can still see some licenses that should be fixed. Regarding the history, external contributions are minimal, so loosing the history should not be a problem; especially because there are a lot of noise in there (including the Hack patch that was later reverted).

fabpot · 2015-09-24T11:35:57Z

ping @symfony/deciders

derrabus · 2015-09-24T11:57:37Z

composer.json

@@ -76,7 +77,11 @@
        "monolog/monolog": "~1.11",
        "ircmaxell/password-compat": "~1.0",
        "ocramius/proxy-manager": "~0.4|~1.0",
-        "egulias/email-validator": "~1.2"
+        "egulias/email-validator": "~1.2",
+        "phpdocumentor/reflection": "~1.0"


You've added a conflict block below for versions < 1.0.7. Shouldn't we change the version constraint instead to ^1.0.7 and drop the conflict block?

https://github.com/symfony/symfony/pull/15858/files#r40200338

Oh, I've just found the other composer.json with the discussion about this topic. I'll join there. Nevermind. ;-)

derrabus · 2015-09-24T13:02:13Z

Let's test this Carson thingy…

Status: Reviewed

derrabus · 2015-09-24T13:02:29Z

🎉

OskarStark · 2015-09-24T13:11:49Z

@carsonbot ...

Status: Reviewed

dunglas · 2015-09-24T17:23:10Z

Issues raised by @derrabus and @OskarStark fixed.

fabpot · 2015-09-25T06:41:29Z

Reading the documentation, here are some thoughts:

The PropertyInfo class name is confusing; it looks like a class that represents a single property;
About the constructor arguments of PropertyInfo: Would it be easier for end users to pass one list of extractors (they would all implement an empty marker interface) and then, depending on the interfaces they implements, we can easily separate them into 4 buckets. The only drawbacks I can see is if the ordering would be different depending on the interface they implement but I doubt it's the case.

dunglas · 2015-09-25T06:55:55Z

Do you have a suggestion for a better name? What do you think about PropertyInfoExtractor?
The ordering matters and it's exactly why arguments they are one argument by type. The first implementation was exactly like you describe but when developing API Platform I found the following particular use case: list of properties must be extracted using reflection but type with the Doctrine extractor and it's not possible without segregating the arguments. IMO it's not a big deal from developper POV because the component will be exposed as a service (I'll provide a PR when this one will be merged) in the full stack framework. Developers using the standalone component probably have enough background to understand the ordering issue.

dunglas · 2015-09-25T09:41:25Z

Another solution for the number of arguments: the constructor as is but provide a factory method with only one parameter (the array of extractors) doing the segregation.

It looks overkill for such a low level component but I can add it if you think it's better in term of DX.

fabpot · 2015-09-25T10:07:27Z

PropertyInfoExtractor sounds good to me.
As the ordering matters, let's keep the current constructor as is.

dunglas · 2015-09-25T12:14:43Z

Class and interfaces renamed accordingly.

fabpot · 2015-09-25T12:22:37Z

👍

fabpot · 2015-09-26T07:46:33Z

Thank you @dunglas.

fabpot · 2015-09-26T07:53:05Z

https://packagist.org/packages/symfony/property-info
https://github.com/symfony/property-info

@dunglas

This PR was merged into the 2.8 branch. Discussion ---------- [PropertyInfo] Add Component Documentation | Q | A | ------------- | --- | Doc fix? | no | New docs? | yes (symfony/symfony#15858) | Applies to | `2.8`, `3.0` | Fixed tickets | N/A This is still a *work in progress*, but I'd like feedback from anyone regarding structure, grammar and any other improvements while I continue. After speaking with @dunglas, I'm creating a new PR due our branches differing by about 400 commits. Commits ------- a0409d7 Update Reference to Service Container Tags 78ea4a5 Update PropertyAccess Component Path 7b786af Fix Incorrect Indent 96b139b Suggested Changes cf7a0ea Add PropertyInfo Component Documentation

carsonbot added the Status: Needs Review label Sep 21, 2015

dunglas reviewed Sep 21, 2015
View reviewed changes

dunglas force-pushed the propertyinfo branch from f6252c3 to e16db41 Compare September 21, 2015 15:15

dunglas mentioned this pull request Sep 21, 2015

Major refactoring to prepare the Symfony merge dunglas/php-property-info#8

Merged

6 tasks

stof reviewed Sep 21, 2015
View reviewed changes

dunglas force-pushed the propertyinfo branch 2 times, most recently from 3fc5195 to a8f643c Compare September 21, 2015 20:43

dunglas mentioned this pull request Sep 22, 2015

Fix #257 uppercase attribute names will stay uppercased api-platform/core#260

Closed

fabpot reviewed Sep 22, 2015
View reviewed changes

dunglas mentioned this pull request Sep 22, 2015

[PropertyInfo] Add the doc symfony/symfony-docs#5717

Closed

dunglas force-pushed the propertyinfo branch from a8f643c to 12339f4 Compare September 22, 2015 15:22

[PropertyInfo] Import the component from dunglas/php-property-info

e696f5d

dunglas force-pushed the propertyinfo branch from 12339f4 to e696f5d Compare September 23, 2015 10:32

derrabus reviewed Sep 24, 2015
View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Sep 24, 2015

Another bunch of CS fixes

6586800

dunglas mentioned this pull request Sep 25, 2015

AttributeMetadata should work with getters too api-platform/core#249

Closed

[PropertyInfo] Rename classes and interfaces

e75d1f1

fabpot closed this in 59ee12c Sep 26, 2015

dunglas deleted the propertyinfo branch October 29, 2015 12:32

fabpot mentioned this pull request Nov 16, 2015

Release v2.8.0-BETA1 #16550

Merged

zanbaldwin mentioned this pull request Dec 7, 2015

[PropertyInfo] Add Component Documentation symfony/symfony-docs#5974

Merged

dunglas mentioned this pull request Dec 7, 2015

[DX] [PropertyInfo] Filter PropertyInfo Extractors with Interfaces #16890

Closed

javiereguiluz mentioned this pull request Dec 18, 2015

Added the documentation of the PropertyInfo component symfony/symfony-docs#6047

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PropertyInfo] Import the component #15858

[PropertyInfo] Import the component #15858

dunglas commented Sep 21, 2015

dunglas Sep 21, 2015

fabpot Sep 22, 2015

mickaelandrieu commented Sep 21, 2015

stof Sep 21, 2015

theofidry commented Sep 21, 2015

dbu commented Sep 21, 2015

stof Sep 21, 2015

dunglas commented Sep 21, 2015

fabpot Sep 22, 2015

fabpot commented Sep 22, 2015

fabpot commented Sep 22, 2015

stof commented Sep 22, 2015

dunglas commented Sep 22, 2015

dunglas commented Sep 22, 2015

fabpot commented Sep 23, 2015

fabpot commented Sep 24, 2015

derrabus Sep 24, 2015

dunglas Sep 24, 2015

derrabus Sep 24, 2015

derrabus commented Sep 24, 2015

derrabus commented Sep 24, 2015

OskarStark commented Sep 24, 2015

dunglas commented Sep 24, 2015

fabpot commented Sep 25, 2015

dunglas commented Sep 25, 2015

dunglas commented Sep 25, 2015

fabpot commented Sep 25, 2015

dunglas commented Sep 25, 2015

fabpot commented Sep 25, 2015

fabpot commented Sep 26, 2015

fabpot commented Sep 26, 2015

[PropertyInfo] Import the component #15858

[PropertyInfo] Import the component #15858

Conversation

dunglas commented Sep 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickaelandrieu commented Sep 21, 2015

Choose a reason for hiding this comment

theofidry commented Sep 21, 2015

dbu commented Sep 21, 2015

Choose a reason for hiding this comment

dunglas commented Sep 21, 2015

Choose a reason for hiding this comment

fabpot commented Sep 22, 2015

fabpot commented Sep 22, 2015

stof commented Sep 22, 2015

dunglas commented Sep 22, 2015

dunglas commented Sep 22, 2015

fabpot commented Sep 23, 2015

fabpot commented Sep 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derrabus commented Sep 24, 2015

derrabus commented Sep 24, 2015

OskarStark commented Sep 24, 2015

dunglas commented Sep 24, 2015

fabpot commented Sep 25, 2015

dunglas commented Sep 25, 2015

dunglas commented Sep 25, 2015

fabpot commented Sep 25, 2015

dunglas commented Sep 25, 2015

fabpot commented Sep 25, 2015

fabpot commented Sep 26, 2015

fabpot commented Sep 26, 2015