symplify icon indicating copy to clipboard operation
symplify copied to clipboard

[monorepo-builder] MBConfig::packageDirectoriesExcludes input validation makes exclusion impossible

Open Kingdutch opened this issue 1 year ago • 2 comments

Problem

MBConfig::packageDirectoriesExcludes performs an Assert::allFileExists($packageDirectories);. This means that paths must either be absolute or relative to the current repository root.

However, Symfony's ExcludeDirectoryFilterIterator will turn any non-simple (i.e. more than one path segment) path into a matcher. The actual matching then happens against a path that is relative to the directories provided to packageDirectories. That's usually already a step removed from the repository root.

This means that the input that is valid for packageDirectoriesExcludes will never match in ExcludeDirectoryFilterIterator.

An example folder structure

  tools/
    oste/
      data/
        composer.json
      composer.json

Desired: Get all packages in tools/*/ but exclude specifically tools/oste/data/composer.json.

Config that passes input validation but doesn't match anything:

  $mbConfig->packageDirectories([
    __DIR__ . '/tools',
  ]);

  $mbConfig->packageDirectoriesExcludes([
    'tools/oste/data',
  ]);

When debugging the matching in ExcludeDirectoryFitlerIterator you will see that oste and oste/data are matched against the exclude input which is tools/oste/data which never matches.

Config that would match the exclude but doesn't pass input validation:

  $mbConfig->packageDirectories([
    __DIR__ . '/tools',
  ]);

  $mbConfig->packageDirectoriesExcludes([
    'oste/data',
  ]);

This doesn't pass input validation because MBConfig::packageDirectoriesExcludes tries to check that oste/data exists (which it doesn't because it lives in tools/oste/data).

Proposed solution

Either modify MBConfig::packageDirectoriesExcludes to remove the folder existence validation. This allows excluding subdirectory patterns in included directories. A downside is that I may want to include directoreis A/ and B/ and exclude B/Foo but not A/Foo which would then not be possible (since the iterator matches only on subpaths).

An alternative is to make sure that ExcludeDirectoryFilterIterator actually gets the path relative to the project root to match on, rather than relative to the include directory. That would allow configuring an exclusion of B/Foo specifically.

Workaround

The deprecated $mbConfig->parameters()->set(Option::PACKAGE_DIRECTORIES_EXCLUDES, []); can be used to work around the faulty input validation in MBConfig.

Kingdutch avatar Sep 06 '22 12:09 Kingdutch

Thanks for sharing the issue in detail.

Could you kick off with a failing PR for your situation? It would be easier to iterate from there.

TomasVotruba avatar Sep 06 '22 12:09 TomasVotruba

@TomasVotruba Here you go: https://github.com/symplify/symplify/pull/4390 :)

Kingdutch avatar Sep 06 '22 15:09 Kingdutch