Skip to content

Feature: add support for $filePaths #2794

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: 2.12.x
Choose a base branch
from

Conversation

rela589n
Copy link

@rela589n rela589n commented Aug 7, 2025

Q A
Type feature
BC Break no

Summary

In the scope of doctrine/persistence#432 (available from doctrine/persistence >= 4.1) there was added
ColocatedMappingDriver::$filePaths, which allows passing an Traversable of file paths for the mapping driver to use.
This commit integrates those changes into AttributeDriver (and AnnotationDriver).

Since doctrine/orm maintains the support for doctrine/persistence of older versions, the AttributeDriver ensures that $filePaths is actually defined. Tests use InstalledVersions to opt into new behaviour if doctrine/persistence is at least version 4.1.

The old behaviour can be adapted into a new one by using DirectoryFilesIterator and FilePathNameIterator on directory paths.

Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

Wouldn't it be possible to keep a single list of paths and split it in two when necessary to have dirs on one side and files on the other? This would simplify use and avoid breaking backward compatibility.

@rela589n
Copy link
Author

rela589n commented Aug 7, 2025

Wouldn't it be possible to keep a single list of paths and split it in two when necessary to have dirs on one side and files on the other? This would simplify use and avoid breaking backward compatibility.

Do you mean passing both directory and file paths into the same list?

@greg0ire suggested the current approach in doctrine/persistence#424 (comment) . This is a way of distinguishing $paths between directory paths and file paths. The old ColocatedMappingDriver only accepted directory paths and RecursiveDirectoryIterator was used to iterate under the hood. The new version is complemented with iterable $sourceFilePathNames, which is an iterable of file paths traversed "as is".

From our vision, ColocatedMappingDriver should accept only an iterable of file paths, and client code should provide an appropriate iterable. The existing directory scan is planned for deprecation. One can choose to use Symfony Finder to specify a more complex logic for finding mapped files, for example.

Or, another approach would be creating a class that represents an iterable of file paths, obtained by expanding the directories in the same way that ColocatedMappingDriver currently does. An instance of this class can be passed in instead of passing in the list of directories. @greg0ire, do you think we can create this class as part of doctrine/persistence package so that people could easily go toward the new way?

To summarize, mixing directory and file paths in the same list would make things more complex. The code would need to handle both types of paths and expand directories into individual files, making the ColocatedMappingDriver logic more complicated. The end goal is opposite - making its interface as simple as an iterable of file paths.

@rela589n rela589n force-pushed the feat-attribute-driver-iterable-files branch from 3c2c72e to 45d0043 Compare August 7, 2025 13:52
@GromNaN
Copy link
Member

GromNaN commented Aug 7, 2025

I was too quick in my first review. I agree it's simpler to have either a list of dir names or a list of file names. But then I would remove the $pathsAsFilePaths parameter and detect it instead.

$pathsAsFilePaths = match(true) {
    is_file($paths[0]) => true,
    is_dir($paths[0]) => false,
    default => throw new RuntimeException
}

And then loop and validate that we only have dirs or file names.
I'm not sure if missing dirs or files are skipped or throw an error, but that can be handled.

@greg0ire
Copy link
Member

greg0ire commented Aug 7, 2025

@GromNaN it would probably be better to discuss this at doctrine/persistence#429 with everybody. I will quote your message and respond to it.

@rela589n rela589n force-pushed the feat-attribute-driver-iterable-files branch 2 times, most recently from aeaaca3 to 06f744e Compare August 8, 2025 13:40
@rela589n rela589n changed the title Feature: add support for $sourceFilePathNames Feature: add support for $filePaths Aug 8, 2025
@rela589n rela589n marked this pull request as draft August 9, 2025 13:41
In the scope of doctrine/persistence#432
(available from `doctrine/persistence` >= 4.1) there was added
`ColocatedMappingDriver::$filePaths`, which allows
passing an `Traversable` of file paths for the mapping driver to use.
This commit integrates those changes into `AttributeDriver`
(and `AnnotationDriver`).

Since `doctrine/orm` maintains the support
for `doctrine/persistence` of older versions, the `AttributeDriver`
ensures that `$filePaths` is actually defined.
Tests use `InstalledVersions` to opt into new behaviour if
`doctrine/persistence` is at least version 4.1.

The old behaviour can be adapted into new by using
`DirectoryFilesIterator` and `FilePathNameIterator` on directory paths.
@rela589n rela589n force-pushed the feat-attribute-driver-iterable-files branch from 06f744e to 6e1add0 Compare August 9, 2025 13:45
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