Scope composer dependencies #6614

m1r0 · 2023-03-07T18:21:26Z

Resolves #6574

Proposed Changes

Use php-scoper to scope the composer dependencies by moving them to a new namespace (Sensei\ThirdParty).
The scoped dependencies live in a new folder (third-party).
Use the standard composer autoloader to load the scoped dependencies. This approach avoids some of the autoloading issues described in How to use this to prefix a WordPress plugin? humbug/php-scoper#442.
Update the CI workflows to use the scoped dependencies.

Testing Instructions

I was not able to add tests for the composer Actions class. The passed Event class is only loaded by composer so I can't mock it. Let me know if you have suggestions on how to test it.

Delete your vendor folder.
Run composer install.
Check that the third-party folder is created and not empty.
Remove the dev dependencies by running composer install --no-dev
Check that your vendor folder has only the composer autoloader related files and no dev dependencies.
Enable the email_customization feature flag.
Go to Sensei LMS -> Settings -> Emails and click "Preview" on one of the emails. This makes sure the emogrifier dependency is loaded and working.
Download the CI generated build zip and make sure it works.
Make sure the composer dev dependencies are not included in the CI generated build zip.

Pre-Merge Checklist

m1r0 · 2023-03-07T18:27:55Z

composer.json

 		"phpcompatibility/phpcompatibility-wp": "2.1.4",
 		"sirbrillig/phpcs-no-get-current-user": "1.1.0",
 		"sirbrillig/phpcs-variable-analysis": "2.11.9",
 		"squizlabs/php_codesniffer": "3.7.1",
 		"wp-coding-standards/wpcs": "2.3.0",
 		"yoast/phpunit-polyfills": "1.0.4"
 	},
+	"minimum-stability": "dev",


Lowering the minimum stability because php-scoper has a dev-master dependency.

m1r0 · 2023-03-07T18:28:16Z

config/scoper.inc.php

+		Finder::create()->files()->in( 'vendor/pelago/emogrifier' )->name( [ '*.php', 'LICENSE', 'composer.json' ] ),
+		Finder::create()->files()->in( 'vendor/sabberworm/php-css-parser' )->name( [ '*.php', 'LICENSE', 'composer.json' ] ),
+		Finder::create()->files()->in( 'vendor/symfony/css-selector' )->name( [ '*.php', 'LICENSE', 'composer.json' ] ),
+		Finder::create()->files()->in( 'vendor/symfony/polyfill-php80' )->name( [ '*.php', 'LICENSE', 'composer.json' ] ),


We use only the emogrifier dependency but its dependencies should also be included.

m1r0 · 2023-03-07T18:30:38Z

.gitignore

@@ -11,6 +11,7 @@ hookdocs
 node_modules/
 tests/images/test-*
 vendor/
+vendor_prefixed/


Folder name suggestions are welcome. 🙂

Maybe libs, deps, vendor_scoped, third-party? 🤔

Having "vendor" in the name is a bit confusing so I've updated the folder/namespace to third-party / Sensei\ThirdParty.

m1r0 · 2023-03-07T18:39:38Z

composer.json

@@ -7,14 +7,39 @@
 		"bamarni/composer-bin-plugin": "1.8.2",
 		"dealerdirect/phpcodesniffer-composer-installer": "0.7.2",
 		"dms/phpunit-arraysubset-asserts": "0.4.0",
+		"humbug/php-scoper": "0.13.9",
+		"pelago/emogrifier": "6.0.0",


The pelago/emogrifier dependency is intentionally moved to require-dev because it shouldn't be loaded by the composer autoloader (vendor/autoload.php). The third-party dependencies are now managed by the php-scoper config and then autoloaded by vendor/autoload.php.

Isn't Emgorifier used in Sensei\Internal\Emails\Email_Sender?
I mean that usually considered as a production dependency.

Hum. I think I understand the problem. We don't include vendor (as a source of dependencies) anywhere, so it doesn't make sense to use production dependencies at all. Basically, I think it is a bit confusing, but we probably "resolve" this confusing by documenting it properly.

Yep, I agree, it's a bit confusing. Another option was to have a separate composer file but I felt that is a bad idea. 🙂 Where do you think would be a good place to add docs for this? CONTRIBUTING.md?

For posterity: the way I look at it is that the third-party dependencies (other projects call them libs) are not part of the composer production dependencies. Composer is there to only generate and autoload them.

Looks like a good place.
Maybe also add information about it here: https://github.com/Automattic/sensei/wiki/Setting-Up-Your-Development-Environment

codecov · 2023-03-07T18:40:15Z

Codecov Report

Merging #6614 (ebbba67) into trunk (902fb3d) will increase coverage by 0.04%.
The diff coverage is n/a.

❗ Current head ebbba67 differs from pull request most recent head 8a20894. Consider uploading reports for the commit 8a20894 to get more accurate results

@@             Coverage Diff              @@
##              trunk    #6614      +/-   ##
============================================
+ Coverage     48.10%   48.15%   +0.04%     
- Complexity     9993     9999       +6     
============================================
  Files           502      502              
  Lines         35578    35602      +24     
  Branches        283      283              
============================================
+ Hits          17115    17144      +29     
+ Misses        18251    18246       -5     
  Partials        212      212

Impacted Files	Coverage Δ
includes/internal/emails/class-email-sender.php	`88.04% <ø> (+0.94%)`	⬆️

... and 2 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0cbeb0a...8a20894. Read the comment docs.

… well

merkushin

Works well both locally and on jurassic site 👍

As for the test for the Actions class, I don't see a straightforward solution here. We can consider a couple of workarounds. One is to add this class (or interface) to tests suite. Another is to remove type hinting and use "duck-typing" approach.

merkushin · 2023-03-12T19:24:01Z

composer.json

@@ -7,14 +7,39 @@
 		"bamarni/composer-bin-plugin": "1.8.2",
 		"dealerdirect/phpcodesniffer-composer-installer": "0.7.2",
 		"dms/phpunit-arraysubset-asserts": "0.4.0",
+		"humbug/php-scoper": "0.13.9",
+		"pelago/emogrifier": "6.0.0",


Hum. I think I understand the problem. We don't include vendor (as a source of dependencies) anywhere, so it doesn't make sense to use production dependencies at all. Basically, I think it is a bit confusing, but we probably "resolve" this confusing by documenting it properly.

m1r0 added 7 commits March 7, 2023 14:11

Add the php-scoper package and update composer.json

e79f9db

Ignore the vendor_prefixed folder

8af7ca2

Add the composer autoloader

d20ca7e

Cleanup old composer autoloaders

0d5e189

Add php-scoper config

25ebb73

Update the code to use the prefixed vendor dependencies

7d78c49

Prefix the dependencies using composer script

6446c32

m1r0 added the No Changelog label Mar 7, 2023

m1r0 self-assigned this Mar 7, 2023

m1r0 changed the title ~~Scope dependencies~~ Scope composer dependencies Mar 7, 2023

m1r0 added the [Type] Maintenance label Mar 7, 2023

m1r0 added this to the 4.12.0 milestone Mar 7, 2023

m1r0 commented Mar 7, 2023

View reviewed changes

Fix linter

554860b

m1r0 force-pushed the add/scope-dependencies branch from 6cde5ec to 554860b Compare March 7, 2023 20:03

Exclude vendor_prefixed from phpcs

d334682

m1r0 force-pushed the add/scope-dependencies branch 4 times, most recently from b2cb014 to b4aa3cb Compare March 7, 2023 22:04

m1r0 removed the No Changelog label Mar 7, 2023

m1r0 force-pushed the add/scope-dependencies branch 2 times, most recently from 4d0f944 to fd8fb5f Compare March 7, 2023 22:22

Fix CI E2E tests by installing composer dependencies

95321ae

m1r0 force-pushed the add/scope-dependencies branch from fd8fb5f to 95321ae Compare March 7, 2023 22:26

Add the prefixed dependencies to the CI build package

7e8848c

m1r0 force-pushed the add/scope-dependencies branch from 2a67c76 to 7e8848c Compare March 7, 2023 22:42

m1r0 added the No Changelog label Mar 7, 2023

Include the vendor and vendor_prefixed in the build archive

5ee9a9c

m1r0 marked this pull request as ready for review March 7, 2023 23:04

m1r0 requested a review from a team March 7, 2023 23:33

Rename vendor_prefixed folder to third-party and the namespace as…

ebbba67

… well

merkushin added the [Status] Needs Documentation Requires documentation updates label Mar 12, 2023

merkushin previously approved these changes Mar 12, 2023

View reviewed changes

Add tests for Sensei\Config\Composer\Actions

8a20894

m1r0 dismissed merkushin’s stale review via 8a20894 March 13, 2023 17:49

m1r0 merged commit 0ae251c into trunk Mar 13, 2023

m1r0 deleted the add/scope-dependencies branch March 13, 2023 17:59

m1r0 mentioned this pull request Mar 13, 2023

Add a build step for the third party dependencies #6638

Merged

19 tasks

m1r0 mentioned this pull request Aug 4, 2023

Add composer package scoping info to the contributing doc #7060

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scope composer dependencies #6614

Scope composer dependencies #6614

m1r0 commented Mar 7, 2023 •

edited

Loading

m1r0 Mar 7, 2023

m1r0 Mar 7, 2023

m1r0 Mar 7, 2023 •

edited

Loading

m1r0 Mar 8, 2023 •

edited

Loading

m1r0 Mar 7, 2023 •

edited

Loading

merkushin Mar 12, 2023 •

edited

Loading

merkushin Mar 12, 2023

m1r0 Mar 13, 2023

merkushin Mar 13, 2023

codecov bot commented Mar 7, 2023 •

edited

Loading

merkushin left a comment

merkushin Mar 12, 2023

Scope composer dependencies #6614

Scope composer dependencies #6614

Conversation

m1r0 commented Mar 7, 2023 • edited Loading

Proposed Changes

Testing Instructions

Pre-Merge Checklist

m1r0 Mar 7, 2023

Choose a reason for hiding this comment

m1r0 Mar 7, 2023

Choose a reason for hiding this comment

m1r0 Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

m1r0 Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

m1r0 Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

merkushin Mar 12, 2023 • edited Loading

Choose a reason for hiding this comment

merkushin Mar 12, 2023

Choose a reason for hiding this comment

m1r0 Mar 13, 2023

Choose a reason for hiding this comment

merkushin Mar 13, 2023

Choose a reason for hiding this comment

codecov bot commented Mar 7, 2023 • edited Loading

Codecov Report

merkushin left a comment

Choose a reason for hiding this comment

merkushin Mar 12, 2023

Choose a reason for hiding this comment

m1r0 commented Mar 7, 2023 •

edited

Loading

m1r0 Mar 7, 2023 •

edited

Loading

m1r0 Mar 8, 2023 •

edited

Loading

m1r0 Mar 7, 2023 •

edited

Loading

merkushin Mar 12, 2023 •

edited

Loading

codecov bot commented Mar 7, 2023 •

edited

Loading