Parent issue: #2337283: Add a composer.json file to every component

Problem/Motivation

Part of the parent parent issue: #1826054: [Meta] Expose Drupal Components outside of Drupal

Each of the \Drupal\Component\ namespaces need their own composer.json file, so that they can eventually be offered to the world as a library.

Proposed resolution

Add the LICENSE.txt, README.txt, TESTING.txt, and composer.json files from the template in this patch: https://www.drupal.org/node/2337283#comment-9706563

Verify the various dependencies of the component, so they can be included as requirements.

Add the component's directory to \Drupal\Tests\ComposerIntegrationTest:: getPaths().

Use composer validate in the component directory to make sure the composer.json file is valid.

Remaining tasks

User interface changes

API changes

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Create a patch Instructions
Reroll the patch if it no longer applies. Instructions
Update the issue summary noting if allowed during the rc Template
Update the patch to incorporate feedback from reviews (include an interdiff) Instructions
Manually test the patch Novice Instructions
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions

Postponed until

Framework manager and BDFL signoff for #1826054: [Meta] Expose Drupal Components outside of Drupal

CommentFileSizeAuthor
#8 interdiff-2600718-1-7.txt991 bytesrakesh.gectcr
#8 2600718-7.patch20.72 KBrakesh.gectcr
#2 2600718-1.patch20.67 KBrakesh.gectcr

Comments

Mile23 created an issue. See original summary.

rakesh.gectcr’s picture

rakesh.gectcr
he/him
Primary language English
Location Manchester
commented
Status: Active » Needs review
StatusFileSize
new 2600718-1.patch20.67 KB
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Component/Bridge/TESTING.txt
@@ -0,0 +1,19 @@
+
+$ cd ./core
+$ ./vendor/bin/phpunit --group Bridge

Ugh. The testing instructions here are wrong, because the template was wrong. It should be:

cd core
../vendor/bin/phpunit
# note .. instead of .

...since that's how it is in the linked instructions.

It'll be like this for all these composer.json issues.

dawehner’s picture

dawehner
Primary language German
commented

The alternative for the testing could be ...

./vendor/bin/phpunit -c core --group Bridge
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

I'd prefer that, but then we'd have to change the docs at https://www.drupal.org/node/2116263 and I'm not sure whether this issue warrants that.

rakesh.gectcr’s picture

rakesh.gectcr
he/him
Primary language English
Location Manchester
commented
Assigned: Unassigned » rakesh.gectcr
rakesh.gectcr’s picture

rakesh.gectcr
he/him
Primary language English
Location Manchester
commented

@dawehner && @Mile23

I have done the above mentioned changes, the changes are 

diff -u b/core/lib/Drupal/Component/Bridge/TESTING.txt b/core/lib/Drupal/Component/Bridge/TESTING.txt
--- b/core/lib/Drupal/Component/Bridge/TESTING.txt
+++ b/core/lib/Drupal/Component/Bridge/TESTING.txt
@@ -19 +19 @@
-$ ./vendor/bin/phpunit --group Bridge
+$ ./vendor/bin/phpunit -c core --group Bridge
diff -u b/core/tests/Drupal/Tests/ComposerIntegrationTest.php b/core/tests/Drupal/Tests/ComposerIntegrationTest.php
--- b/core/tests/Drupal/Tests/ComposerIntegrationTest.php
+++ b/core/tests/Drupal/Tests/ComposerIntegrationTest.php
@@ -49,11 +49,11 @@
     return [
       $this->root,
       $this->root . '/core',
+      $this->root . '/core/lib/Drupal/Component/Bridge',
       $this->root . '/core/lib/Drupal/Component/Gettext',
       $this->root . '/core/lib/Drupal/Component/Plugin',
       $this->root . '/core/lib/Drupal/Component/ProxyBuilder',
       $this->root . '/core/lib/Drupal/Component/Utility',
-      $this->root . '/core/lib/Drupal/Component/Bridge',
     ];
   }

If this is ok , I will update other similar issues as well? Ones after the confirmation from you guys.

rakesh.gectcr’s picture

rakesh.gectcr
he/him
Primary language English
Location Manchester
commented
Status: Needs work » Needs review
StatusFileSize
new 2600718-7.patch20.72 KB
new interdiff-2600718-1-7.txt991 bytes
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Reviewed & tested by the community

Looks good. It's still different from the documentation we're linking to, but let's allow a maintainer figure out if it's a big deal. :-)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: 2600718-7.patch, failed testing.

rakesh.gectcr’s picture

rakesh.gectcr
he/him
Primary language English
Location Manchester
commented
Status: Needs work » Reviewed & tested by the community
xjm’s picture

xjm
she/her
Primary language English
commented

I believe this is 8.1.x material now. Thanks for the patch!

xjm’s picture

xjm
she/her
Primary language English
commented
Version: 8.0.x-dev » 8.1.x-dev
dawehner’s picture

dawehner
Primary language German
commented

I believe this is 8.1.x material now. Thanks for the patch!

Can you explain why this is 8.1.x material? IMHO exposing our libraries doesn't cause any change for runtime code.

dawehner’s picture

dawehner
Primary language German
commented

Ah I guess things are just not explained properly on https://www.drupal.org/core/d8-allowed-changes#minor I guess.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Version: 8.1.x-dev » 8.0.x-dev

It is a completely non-disruptive change which allows for greater use of the code by everyone.

This is a step along the way towards a subtree split which allows the PHP community at large to use the code, find bugs, and fix them.

There are tests in place to check the veracity of the composer.json file, and in fact the patch adds the file to the testing regime.

It's not a feature request.

See the parent issue where wide community support is in evidence: #2337283: Add a composer.json file to every component

This novice implementation detail issue is not the place to discuss which branch it should be in. That conversation should happen here: #1826054: [Meta] Expose Drupal Components outside of Drupal

xjm’s picture

xjm
she/her
Primary language English
commented
Version: 8.0.x-dev » 8.1.x-dev

Please review https://www.drupal.org/core/d8-allowed-changes#patch. According to current policy, the only changes specifically allowed for patch releases are:

  • criticals, if the impact outweighs the disruption
  • bug fixes
  • API documentation improvements
  • patch-level library updates
  • issues for backport to D7 that meet the above criteria

This is not any of these things. Moving back to 8.1.x. This doesn't mean committing it would be postponed for ages, just that we are not deploying it to production sites in two weeks.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Needs framework manager review

Also tagging for framework manager review. I'll tag the parent that way as well.

xjm’s picture

xjm
she/her
Primary language English
commented
Assigned: rakesh.gectcr » Unassigned
Issue tags: -Needs framework manager review

@Mile23 is right that we should make sure to keep the discussion in the meta issue. Based on that, I'm actually postponing this pending framework manager and BDFL review of the overall plan. Once that's there we can set this issue back to RTBC against 8.1.x. Thanks!

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes
Status: Reviewed & tested by the community » Postponed
xjm’s picture

xjm
she/her
Primary language English
commented

(Fixing issue credit to include @Mile23 and @dawehner.)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Postponed » Reviewed & tested by the community

Including composer.json files has been approved by Dries in #2176065-33: Introduce a composer.json for Drupal\Component\Utility. Discussed with @xjm and we agreed that these tasks should be committed to 8.1.x

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed d62a0a6 and pushed to 8.1.x. Thanks!

  • alexpott committed d62a0a6 on 8.1.x 
    Issue #2600718 by rakesh.gectcr, xjm, Mile23, dawehner: Add composer....

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.