Feature: iterable source file path names for ColocatedMappingDriver

[rela589n]

This PR follows from #424.

It aims to rework ColocatedMappingDriver so that it won't have the responsibility of scanning for files in the provided directories, but will instead accept an iterable of source file path names. Thus, every individual component using ColocatedMappingDriver should provide a complete iterator that covers all files.

Ultimately, Symfony Finder should be used to grab a list of PHP files that must be mapped. This aims for more flexibility regarding file scanning, so that users would be able to customize what files to include.

[greg0ire]

Why are there 9 commits? Please make fewer commits, with better messages.
See the contributing guide.

[greg0ire]

Thanks @rela589n !

[rela589n]

Thank you, @greg0ire

So, the depending libraries need to be updated now.
I'm sure that doctrine/orm is the first to be updated to allow passing an iterable into AttributeDriver, so I plan to do this next.

Do you have any considerations about how to do this properly?

[greg0ire]

Do something similar to what you've done in your test, then contribute to DoctrineBundle a service (could be based on a symfony/finder) that does exactly what the legacy code does. It should also possible to provide another service instead, so that you can finally reach your personal goal.

I guess the same needs to be done on the ODM part.

Then you can add deprecations.

[rela589n]

Do something similar to what you've done in your test

Sure! The only question I have right now is how to make things mesh together, since the changes made to doctrine/persistence haven't been released yet, and the changes to doctrine/orm would require them. I could temporarily "require-dev": { "doctrine/persistence": "4.1.x-dev"} to make it work, but what is the correct way of doing this? If require-dev is acceptable, I can proceed with this.

that does exactly what the legacy code does

So, you mean the service that can accept an array of paths, and will implement iterable<string> that is needed for AttributeDriver?

It should also possible to provide another service instead, so that you can finally reach your personal goal.

If I understand correctly, it could be achieved via a Compiler Pass

[greg0ire]

Sure! The only question I have right now is how to make things mesh together, since the changes made to doctrine/persistence haven't been released yet, and the changes to doctrine/orm would require them. I could temporarily "require-dev": { "doctrine/persistence": "4.1.x-dev"} to make it work, but what is the correct way of doing this? If require-dev is acceptable, I can proceed with this.

You can start like this, then before merging we release 4.1.0. That way, if a breaking change needs to be made, we can still do it before releasing.

So, you mean the service that can accept an array of paths, and will implement iterable that is needed for AttributeDriver?

Right now, there is a way to configure the paths through config, right? Well that service should be able to reuse that config to do the same thing using the new way.

[greg0ire]

From @GromNaN in doctrine/mongodb-odm#2794 (comment):

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.

The iterable doesn't necessarily implement ArrayAccess, but maybe it's still possible to do something like this:

$pathsAsFilePaths = false;
$first = true;
foreach ($paths as $path) {
    if ($first && is_file($path)) {
        $pathsAsFilePaths = true;
    }
}

A bit weird, but if it allows not having to change the method signature, it sounds great!

[greg0ire]

Extra question: do you think there might be cases where people would need to call addPaths() with an iterable of files, then call it again with an array of directories? I'm thinking that might be the case if there is some mapping configuration provided by a bundle like FOSUserBundle

The answer will impact #430

I'm starting to think the best implementation could be to do something like this

foreach ($paths as $path) {
    switch(true) {
        case is_file($path):
            $this->newLogic($path);
            break;

        case is_dir($path):
            $this->legacyLogic($path);
            break;

        default:
            throw new \InvalidArgumentException(sprintf(
                '"%s" is neither a file nor a directory.',
                $path,
            ));
    }
}

legacyLogic() could call newLogic() with what it found inside the directory (don't actually name the methods like this please) .

A side effect of this is that it would allow iterable<dir|file> when before we only allowed iterable<dir>|iterable<file>. I'm not saying it's useful, but it would certainly be a side effect.

[rela589n]

Don't you think that this will complicate the interface? iterable<dir|file> is harder to deal with, harder to deprecate, harder to migrate. People who'd pass iterable<dir|file> would need to rework it toward iterable<file>, which is not that straightforward.

If it be so, why would we not allow either array<dir> (as currently) or iterable<file>? It's possible to check the type of iterable - whether it's a file or a directory, similar to the proposed:

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

With the difference that it doesn't require ArrayAccess:

$pathsAsFilePaths = true;

foreach ($paths as $path) {
    if (!is_file($path)) {
        $pathsAsFilePaths = false;
    }

    break;
}

Since we plan to migrate toward the iterable of files, IMO we should not allow iterable<dir|file>, lest people start using it this way.

[greg0ire]

Makes sense. So while it would be technically possible with the code I mentioned, let's not document that it's possible. Do you have thoughts about my FOSUserBundle question?

[rela589n]

Extra question: do you think there might be cases where people would need to call addPaths() with an iterable of files, then call it again with an array of directories? I'm thinking that might be the case if there is some mapping configuration provided by a bundle like FOSUserBundle

Can't see any addPaths() there https://github.com/search?q=repo%3AFriendsOfSymfony%2FFOSUserBundle+%22addPaths%22&type=code

Anyway, if the driver uses an iterable of files, there's no need for files to be added via addPaths(). If they need to do so, they'd be able to pass the mutable iterable. When using Symfony Finder, one could modify the Finder after it has been created.

One more question I have, though, is whether https://github.com/doctrine/DoctrineBundle is going to be updated so that iterable is the new default? Do we check for property_exists(AttributeDriver::class, 'sourceFilePathNames'), like it was in AttributeDriver, or check for the installed version instead?

[greg0ire]

Yes, I think iterable should be the new default, and that we should use property_exists for consistency's sake.

[rela589n]

should use property_exists

Understand. I asked because it's private, and I was hesitant to check if the property exists outside of the class.

So, what's next? Should I go and implement the driver without any extra arguments?

[greg0ire]

So, what's next? Should I go and implement the driver without any extra arguments?

Yes. If you want me to, I can revert #429 (or maybe only a6cb5a1), then you can submit something with a revert of the revert, your modifications, and modifications from #430, it might be easier for everyone to review.

[rela589n]

Let's do it

[greg0ire]

#431

[GromNaN]

I think iterable should be the new default.

Yes!

When setting the $this->paths array, an iterators should be created internally for backward compatibility.

The constructor will accept string[]|Iterator<string>.

• string[] is a list of dir names, it should trigger a deprecation when this is used.
• Iterator<string> is a list of file names.

The methods addPaths, getPaths, addExcludePaths, getExcludePaths, getFileExtension, setFileExtension will be deprecated and throw an exception when an iterator has been set.

In the end, this trait should perhaps be totally depreciated.

[greg0ire]

In the end, this trait should perhaps be totally depreciated.

In favor of a new trait? In any case, let us add the deprecations after the O[DR]M and bundles have been adapted to the new code please.

[rela589n]

The constructor will accept string[]|Iterator.

That's an interesting statement. Using Iterator in favor of iterable has the benefit that we won't need to check the type of iterable manually (dir vs file). We can always know that the Iterator is the Iterator of file paths.

this trait should perhaps be totally depreciated.

Didn't get this either

[greg0ire]

The constructor will accept string[]|Iterator.

That's an interesting statement. Using Iterator in favor of iterable has the benefit that we won't need to check the type of iterable manually (dir vs file). We can always know that the Iterator is the Iterator of file paths.

Yeah but it will make the new way harder to use in trivial cases, which include tests.

this trait should perhaps be totally depreciated.

Didn't get this

I think @GromNaN means you could create a new trait instead of modifying the existing one.

[rela589n]

Yeah but it will make the new way harder to use in trivial cases, which include tests.

It's as simple as creating a new object.

return new MyDriver(new ArrayIterator($filePaths));