Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[PropertyInfo] Import the component#15858
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 commentedSep 21, 2015
👍 this is realy useful |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
we generally use singular for namespaces in Symfony
theofidry commentedSep 21, 2015
👍 |
dbu commentedSep 21, 2015
what about the doctrine variants (mongo, phpcr)? can they provide their own extractor in their symfony bundle? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this supports string keys too
dunglas commentedSep 21, 2015
@dbu IMO it could even be added directly in the Doctrine bridge. |
3fc5195 toa8f643cCompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You should mention that you also need the Doctrine bridge.
fabpot commentedSep 22, 2015
Can you rebase to get rid of all the merge commits that we don't want to keep when merging? |
fabpot commentedSep 22, 2015
LGTM |
stof commentedSep 22, 2015
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 commentedSep 22, 2015
dunglas commentedSep 22, 2015
The AppVeyor error is unrelated. |
fabpot commentedSep 23, 2015
@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 commentedSep 24, 2015
ping @symfony/deciders |
composer.json Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You've added aconflict block below for versions < 1.0.7. Shouldn't we change the version constraint instead to^1.0.7 and drop theconflict block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh, I've just found the other composer.json with the discussion about this topic. I'll join there. Nevermind. ;-)
derrabus commentedSep 24, 2015
Let's test this Carson thingy… Status: Reviewed |
derrabus commentedSep 24, 2015
🎉 |
OskarStark commentedSep 24, 2015
@carsonbot ... Status: Reviewed |
dunglas commentedSep 24, 2015
Issues raised by@derrabus and@OskarStark fixed. |
fabpot commentedSep 25, 2015
Reading the documentation, here are some thoughts:
|
dunglas commentedSep 25, 2015
|
dunglas commentedSep 25, 2015
Another solution for the number of arguments: the constructor as is but provide a factory method with only one parameter (the 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 commentedSep 25, 2015
|
dunglas commentedSep 25, 2015
Class and interfaces renamed accordingly. |
fabpot commentedSep 25, 2015
👍 |
fabpot commentedSep 26, 2015
Thank you@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/AThis 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 Tags78ea4a5 Update PropertyAccess Component Path7b786af Fix Incorrect Indent96b139b Suggested Changescf7a0ea Add PropertyInfo Component Documentation
As discussed with@fabpot (see#14844), this PR movesdunglas/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:
Usage:
Output: