We want Doctrine Common so that the Plugins system can continue work. This also updates to Symfony 2.1.0-beta2.
Steps I took to generate the patch:
- Added
"doctrine/common": "2.2.*"
to composer.json - In /core, I ran
drush dl composer drush composer update
This would alternatively work:
curl -s http://getcomposer.org/installer | php php composer.phar update
- Doctrine was installed and Symfony was updated
- Added the Doctrine namespace definition to drupal_classloader()
This is why we want this RTBC patch in, so that there are no conflicts when namespace updates are added. Can we please get that committed so that issues stopped being blocked?
More on Doctrine:
Doctrine Common is a library that provides a class autoloader and a class annotations reader. We are looking to rely on Doctrine for extracting plugin definitions from class annotations, though its class loader may have some potential as well (I don't know, we already seem to have one of those). There are a variety of other annotation parsing libraries out there. I have had at least a half a dozen thrown at me since we started using doctrine, however, the clear message from those I asked to advise me in this regard was Doctrine, and it does seem to share a lot of traction with Symfony (which uses Doctrine Annotations for its routing -- we use our own routing so it was not needed yet) which we are also heavily embracing. All things considered, this seems a good fit for us as a community and we already have a plugin discovery method using it that can be found here: http://drupal.org/node/1441840/git-instructions/8.x-annotations
chx has provided additional information that weighs heavily in Doctrine's favor: http://www.slideshare.net/rdohms/annotating-with-annotations
Comment | File | Size | Author |
---|---|---|---|
#29 | doctrine-attribution.patch | 594 bytes | effulgentsia |
#22 | 1683046-22.patch | 634.55 KB | effulgentsia |
#22 | 1683046-22-src.txt | 1022 bytes | effulgentsia |
#19 | 1683046-17-src.txt | 1.68 KB | effulgentsia |
#17 | 1683046-7-src.txt | 5.42 KB | effulgentsia |
Comments
Comment #2
Crell CreditAttribution: Crell commentedTwig 1.9 was just released: http://blog.twig.sensiolabs.org/post/27129940359/twig-1-9-0-released
We should include that in here as well while we're at it.
Comment #3
RobLoachAdded ATrait.php to .gitignore and updated to twig 1.9.
Comment #5
RobLoachSwitched to *Trait.php in gitignore.
Comment #7
RobLoach#1565094: Remove "vendor" directory from PHP syntax check to avoid PHP 5.4 errors
Comment #8
EclipseGc CreditAttribution: EclipseGc commented+1 to adding Doctrine Common, Plugin Annotations needs it.
Comment #9
chx CreditAttribution: chx commentedhttp://www.slideshare.net/rdohms/annotating-with-annotations slide 68 has annotation engines. Slide 69 says Doctrine 2.3: very mature. That's one of the reasons to pick Doctrine.
Comment #10
webchickWhat is Doctrine [Common]?
Why do we want to use it?
Why is it better than whatever alternatives might exist? (It sounds like #9 answers this)
Please update the issue summary with this info.
Comment #10.0
EclipseGc CreditAttribution: EclipseGc commentedAdding detains on Doctrine Common
Comment #11
EclipseGc CreditAttribution: EclipseGc commentedComment #11.0
EclipseGc CreditAttribution: EclipseGc commentedadding slideshare link from chx
Comment #11.1
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #12
Damien Tournoud CreditAttribution: Damien Tournoud commentedDoctrine Common is definitely the "go-to" for annotation parsing. Even the Zend Framework community is discussing using this implementation instead of re-inventing the wheel.
Let's get this in and implement annotation goodies for plugins :)
Comment #13
bojanz CreditAttribution: bojanz commentedDoctrine Common contains generic libraries used by Doctrine.
Basically, it has annotations and (another) class loader.
The annotations library provided there is the only complete and pretty much standard annotations library for PHP.
I fully support this (and have been cheering for Doctrine annotations since before plugins started using them)
Comment #14
RobLoachSymfony moves WAY faster than Drupal. http://symfony.com/blog/symfony-2-1-0-beta3-released .... This patch would still be much easier to look at (and safer) with #1663404: Use Composer's defined namespaces to ease maintenance.
Comment #15
EclipseGc CreditAttribution: EclipseGc commentedThis isn't actually dependent on the composer class loader issue, it just makes not having to do this again easier, and plugins really needs to keep moving on annotations. Moving this back to RTBC, there's no reason to block it best as I can tell.
Eclipse
Comment #16
effulgentsia CreditAttribution: effulgentsia commentedI tried to reroll #7 for Symfony beta3, but when I try to replicate the patch by following the steps in the issue summary, Composer wants to put Doctrine into
core/vendor/doctrine/common/doctrine-common-ac6c39b
, whereas #7 has it in simplycore/vendor/doctrine/common
, and the issue summary doesn't include info on how to tell composer to remove that last directory.Comment #17
effulgentsia CreditAttribution: effulgentsia commentedI cleaned up my workspace and that fixed #16.
Below are attached 3 files:
- Just the minimal hunks needed from #7. That is all that is needed to apply to HEAD prior to running
php composer.phar update
.- A patch with that minimal diff plus after having run
php composer.phar update
.- The interdiff between #7 and the above patch. It only contains changes to composer.lock, installed.json, and the Symfony code differences between beta2 to beta3.
If bot passes, I think this can be RTBC again, though someone verifying that the installed.json changes are ok would be nice: I think it's mostly due to just different ordering and timestamps.
Comment #18
effulgentsia CreditAttribution: effulgentsia commentedJust pointing out the LGPL license of Doctrine Common. I'm guessing this is ok, but I leave it to a core committer to decide.
Comment #19
effulgentsia CreditAttribution: effulgentsia commented#17's 1683046-7-src.txt file contains more than needed. Here's all that's really needed, and
php composer.phar update
does the rest. #17's full patch file is still correct.Comment #21
tstoecklerThis should update core's COPYRIGHT.txt to reference the Doctrine license.
Comment #22
effulgentsia CreditAttribution: effulgentsia commentedGiven the failures in #17, let's make this issue about adding Doctrine Common only, and do Symfony and Twig updates separately.
Here's both the full patch and a txt file of just the manual changes. The full patch can be generated by applying the txt file, then:
We don't reference licenses in our COPYRIGHT.txt file, but we should give attribution per the requirements of the LGPL license. However, I can't find the proper attribution to use. Seems like I'm not the only one.
Comment #23
RobLoach1683046-22-src.txt is proof why #1663404: Use Composer's defined namespaces to ease maintenance is a good idea. Looks good!
Comment #24
catchOK since this is just adding the library and there's already a specific use case for it I've gone ahead and committed this.
However:
- this will need a change notification
- there's no link here to the actual issue trying to use annotations for plugins, for people who also wonder where that is, it's here: #1683644: Use Annotations for plugin discovery.
- looks like there needs to be a follow-up issue to figure out the attribution issue, or we should leave this one open as a critical until that's fixed so we're not breaching the license and pissing people off.
- since the plugins patch currently relies on an implementation detail of the Symfony classloader I have no idea why switching to the Composer autoloader which doesn't have that implementation detail at all would have made this process easier.
Comment #25
jhodgdonOK, I see that "attribution" is listed as a To Do here... At a minimum, we need to get this added to COPYRIGHT.txt.
Comment #26
effulgentsia CreditAttribution: effulgentsia commentedFYI: I emailed the Doctrine Common authors reported by Composer in the installed.json file asking for an appropriate attribution line. Still waiting on a response, but will submit a patch when I get one.
Comment #27
Dries CreditAttribution: Dries commentedI'm not actually convinced we need this in Drupal 8: see http://drupal.org/node/1683644#comment-6257294. At least not for plugins.
Comment #28
catchThat's fine, if we don't use it, we can just remove the library again, but I'd rather have that discussion in the actual plugin issue and keep to the pattern of committing libraries separately to keep patches reviewable.
Tagging this with 'revisit before release' so we don't forget to remove it if it ends up being dead code.
Comment #29
effulgentsia CreditAttribution: effulgentsia commentedMy hunch is we'll end up using the library for at least some, and maybe all, plugins by the time D8 ships, because we'll find it helps performance, but we may not be able to prove that until we have Blocks, various Views plugins, and other plugins in core without it. While we're in this limbo, I'd rather we keep the library in, though that's ultimately for Dries and catch to decide. If we're leaving it in provisionally, here's the attribution requested by Guilherme Blanco, who checked with the other authors prior to sending it to me.
Comment #30
jhodgdonThanks! I've committed this well-formatted patch to the copyright file.
It looks like we still need a change notification?
Comment #31
jhodgdonAnd I guess since the attribution issue is fixed, this goes back to "major", see #24.
Comment #32
tstoecklerComment #33
tstoecklerSorry, crosspost.
Restoring issue properties.
Comment #34
tim.plunkettChange notices are critical tasks.
Comment #35
sunUm. A change notice? Announcing what change, exactly? That we dumped another library into core, but that doesn't affect you nor anyone else, because it didn't previously exist in the first place? ;)
Unless we radically changed the purpose of change notices recently, I'd vote to revert this to fixed. (Though the API addition|change|clean-up tags are always helpful to stream notifications to @drupalchanges!)
Comment #36
Crell CreditAttribution: Crell commentedIs there an index anywhere of all these twitter accounts that have cropped up? AFAIK there's no documentation of what tags we're "supposed" to put where when to feed them, nor a discussion about whether or not we should even have them.
I would agree, no change notice is needed at this time. If we actually make use of it later, we can document it at that time.
Comment #37
tim.plunkettComment #39
xjmComment #39.0
xjmUpdated issue summary.
Comment #40
catchWe are using this for annotations, untagging.