Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 2, 2025

Implement PHP dependency scoping (without HTMLPurifier)

Based on PR #3384 and #3101, implementing dependency scoping to avoid conflicts with other Nextcloud apps. HTMLPurifier has been removed from master and replaced with symfony/html-sanitizer, making the implementation simpler.

Summary

This PR implements PHP dependency scoping using php-scoper to isolate all vendor dependencies under the OCA\News\Vendor namespace. This prevents conflicts when multiple Nextcloud apps use different versions of the same library.

Fixed Issues

  • Composer script syntax: Changed from @composer bin (which doesn't work) to direct cd vendor-bin/php-scoper && composer install
  • Namespace stripping: Fixed escaping in composer.json from OCA\\\\\\\\News\\\\\\\\Vendor to "OCA\\\\News\\\\Vendor" for proper namespace prefix stripping
  • Directory structure: Scoped classes now correctly placed in lib/Vendor/FeedIo/ instead of lib/Vendor/OCA/News/Vendor/FeedIo/
  • Code coverage: Excluded lib/Vendor from code coverage to prevent errors when analyzing scoped third-party dependencies

What's Included

✅ Configuration Files

  • scoper.inc.php: Configures php-scoper to prefix dependencies
  • lib-vendor-organizer.php: Organizes scoped code into lib/Vendor/
  • vendor-bin/php-scoper/composer.json: Declares php-scoper dependency

✅ Build Integration

  • Added bamarni/composer-bin-plugin to composer.json
  • Created scope-dependencies composer script (now working!)
  • Updated Makefile to run scoping automatically after composer install
  • Updated Makefile unit-test to check for scoped dependencies
  • Updated phpstan.neon.dist to exclude lib/Vendor/*
  • Updated phpunit.xml to exclude lib/Vendor/* from code coverage
  • Updated .gitignore to exclude lib/Vendor/*

✅ Code Updates

All vendor imports updated to use OCA\News\Vendor\* namespace:

  • FeedIo* → OCA\News\Vendor\FeedIo*
  • Favicon* → OCA\News\Vendor\Favicon*
  • fivefilters* → OCA\News\Vendor\fivefilters*
  • League\Uri* → OCA\News\Vendor\League\Uri*
  • GuzzleHttp\Psr7* → OCA\News\Vendor\GuzzleHttp\Psr7*
  • Http* → OCA\News\Vendor\Http*

✅ Guzzle Compatibility

  • Added PSR-7 response wrapping in FeedIoClient (Guzzle is not scoped, provided by Nextcloud)
  • Tests updated with mock methods for response wrapping

✅ Appstore Package

  • Updated appstore target to copy scoped dependencies in lib/Vendor/
  • Removes original vendor files (now scoped)
  • Keeps vendor/autoload.php and vendor/composer/ for autoloading
  • Includes composer.json for autoload configuration
  • Cleans up test files and .git directories

✅ Documentation

  • Created docs/dependencies-scoping.md with usage and implementation details

Benefits

No Conflicts - Apps can use different versions of the same library
Independent - News doesn't affect other apps
Simpler than #3384 - No HTMLPurifier special handling needed
Maintainable - Clear namespace prefixing pattern
Standard - Uses php-scoper, a well-maintained tool

Testing

  • All lib/ and tests/ files updated to use scoped namespaces
  • phpcs passes
  • Scoping works cleanly with composer scope-dependencies
  • Makefile build process works
  • PSR-7 response wrapping tested
  • Code review completed and issues addressed
  • Autoloading verified to work correctly
  • Code coverage excludes scoped dependencies

Related PRs

Original prompt

Create a PR that implements dependency scoping for php.

For information you can take PR #3384 and #3101.

Htmlpurifier was removed in master, it was difficult to scope since it is not psr4 complicant and has been replaced now.

So the approach to scoping should be much easier.

Make sure tests are adjusted.
Make sure the makefile is adjusted to execute the scoping.
Make sure that the appstore make command does the right steps to copy all the files. Check carefully what was done in #3384 to make the autoloader stuff work


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI changed the title [WIP] Implement dependency scoping for PHP Implement PHP dependency scoping to prevent library conflicts Nov 2, 2025
Copilot AI requested a review from Grotax November 2, 2025 13:27
@Grotax
Copy link
Member

Grotax commented Nov 2, 2025

Copy link
Contributor Author

Copilot AI commented Nov 2, 2025

@copilot Tests failed: https://github.com/nextcloud/news/actions/runs/19012942385/job/54297099098?pr=3397

Fixed in a79561e. The issue was:

  1. Composer script syntax: The @composer bin command wasn't working, changed to direct cd vendor-bin/php-scoper && composer install
  2. Namespace escaping: Fixed from OCA\\\\\\\\News\\\\\\\\Vendor (8 backslashes) to "OCA\\\\News\\\\Vendor" (with quotes, 4 backslashes) for proper namespace prefix stripping

The scoped dependencies are now correctly placed in lib/Vendor/FeedIo/ instead of lib/Vendor/OCA/News/Vendor/FeedIo/, and autoloading works properly with the PSR-4 mapping "OCA\\News\\": "lib/".

@Grotax
Copy link
Member

Grotax commented Nov 2, 2025

@copilot the unit tests run ok now but

Generating code coverage report in Clover XML format ... Interface "OCA\News\Vendor\Psr\Http\Message\RequestFactoryInterface" not found

Can that be fixed? If not you can also disable the report we do not use it

Copy link
Contributor Author

Copilot AI commented Nov 2, 2025

@copilot the unit tests run ok now but

Generating code coverage report in Clover XML format ... Interface "OCA\News\Vendor\Psr\Http\Message\RequestFactoryInterface" not found...

Fixed in 8e2592d. Added <directory suffix=".php">./lib/Vendor</directory> to the coverage exclusions in phpunit.xml. The scoped third-party dependencies don't need code coverage analysis since they're external libraries.

@Grotax Grotax force-pushed the copilot/implement-dependency-scoping-php branch from 8e2592d to b5fe954 Compare November 9, 2025 10:10
@codecov
Copy link

codecov bot commented Nov 9, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 55.34%. Comparing base (0fd8cd0) to head (27ce050).
⚠️ Report is 7 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3397   +/-   ##
=======================================
  Coverage   55.34%   55.34%           
=======================================
  Files          38       38           
  Lines        1507     1507           
  Branches      311      311           
=======================================
  Hits          834      834           
  Misses        551      551           
  Partials      122      122           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Grotax Grotax force-pushed the copilot/implement-dependency-scoping-php branch from eeecfe6 to f1b6955 Compare November 9, 2025 11:14
@Grotax Grotax requested a review from Copilot November 9, 2025 11:15
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces PHP dependency scoping to isolate the News app's dependencies and prevent conflicts with other Nextcloud apps. All vendor dependencies are prefixed with OCA\News\Vendor namespace and relocated to lib/Vendor/.

Key changes:

  • Implements php-scoper to prefix all dependency namespaces
  • Updates all import statements across the codebase to use scoped namespaces
  • Adds response wrapping in FeedIoClient to bridge unscoped Guzzle responses with scoped feed-io

Reviewed Changes

Copilot reviewed 26 out of 29 changed files in this pull request and generated no comments.

Show a summary per file
File Description
vendor-bin/php-scoper/composer.{json,lock} Adds isolated php-scoper dependency for namespace prefixing
scoper.inc.php Configuration for php-scoper defining prefix and exclusions
lib-vendor-organizer.php Script to reorganize scoped dependencies into lib/Vendor/
composer.json Adds bamarni/composer-bin-plugin and scope-dependencies script
Makefile Integrates scoping into build, test, and appstore targets
lib/**/*.php Updates imports to use OCA\News\Vendor namespace prefix
lib/Fetcher/Client/FeedIoClient.php Adds wrapResponse() to bridge unscoped and scoped PSR-7 responses
tests/**/*Test.php Updates test imports and adds mock methods for wrapped responses
phpunit.xml, phpstan.neon.dist Excludes lib/Vendor from coverage and analysis
.gitignore Ignores generated lib/Vendor directory
docs/dependencies-scoping.md Comprehensive documentation of scoping implementation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Grotax Grotax force-pushed the copilot/implement-dependency-scoping-php branch from 27ce050 to 7a09643 Compare November 13, 2025 18:15
@Grotax Grotax force-pushed the copilot/implement-dependency-scoping-php branch from 7a09643 to 636da21 Compare November 13, 2025 18:19
@Grotax Grotax marked this pull request as ready for review November 13, 2025 18:19
* Fix code style issues in lib-vendor-organizer.php
* Fix scope-dependencies script to properly install and invoke php-scoper
* Exclude lib/Vendor from code coverage to prevent missing interface errors
* fix: Use scoped Symfony HtmlSanitizer classes and clean vendor dir before build
* fix: update verison and changelog
* fix: add link to documentation

Signed-off-by: Benjamin Brahmer <[email protected]>
@Grotax Grotax force-pushed the copilot/implement-dependency-scoping-php branch from 636da21 to 7ff338d Compare November 13, 2025 18:26
@Grotax Grotax merged commit b96639b into master Nov 13, 2025
26 of 27 checks passed
@Grotax Grotax deleted the copilot/implement-dependency-scoping-php branch November 13, 2025 18:30
Grotax added a commit that referenced this pull request Nov 13, 2025
Changed
- Scope PHP dependencies to prevent conflicts with other Nextcloud apps (#3397)

Signed-off-by: Benjamin Brahmer <[email protected]>
@Grotax Grotax mentioned this pull request Nov 13, 2025
Grotax added a commit that referenced this pull request Nov 13, 2025
Changed
- Scope PHP dependencies to prevent conflicts with other Nextcloud apps (#3397)

Signed-off-by: Benjamin Brahmer <[email protected]>
@blizzz
Copy link
Member

blizzz commented Nov 13, 2025

Ah, cool that this is done now. I think I was stuck with some autoloading buts, or dependencies that had pure functions? Sorry for not getting back to it again 😅 But at least it was of use 🙂

Microsoft could have been respectful though in keeping attributions of my code by me, e.g. as base commit…

@Grotax
Copy link
Member

Grotax commented Nov 14, 2025

It was a great starting point and also helped me to understand that htmlpurifier was the blocker in the end. Because the first AI iteration based on your PR almost worked except htmlpurifier 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants