Feature: ColocatedMappingDriver::$fileRegex

[rela589n]

Hi!

I would like to ask for one more review of the proposed feature. In this MR I've simplified it all to the minimal, taking into account previous comments about complex regex.

Please, review it, and feel free to ask if there are any questions :slightly_smiling_face: . Thank you!

[rela589n]

Hi, @greg0ire , could you please check it?

[rela589n]

@greg0ire , just a gentle reminder that I'm still waiting

[greg0ire]

I don't see how this addresses the concerned voiced in https://github.com/doctrine/persistence/pull/417#issuecomment-2827163228 or follow the recommendation made in https://github.com/doctrine/persistence/pull/417#issuecomment-2834991012

[rela589n]

I reopened the PR, because I couldn't get the response

[rela589n]

I agree that using Symfony Finder would be better for complex use cases. It's great component and I appreciate it. But for a simple use case like mine, isn't it a big change for a small gain?

[rela589n]

@greg0ire , what can we do to move on?

[rela589n]

@greg0ire , just a polite nudge :slightly_smiling_face: can we move onward?

[greg0ire]

Not unless you move from the regex implementation to the iterable files implementation.

[rela589n]

Yes, to move with this I need to understand your vision and how you anticipate this.

[rela589n]

@greg0ire , Hi, could you please respond?

[greg0ire]

No.

[rela589n]

Where do you expect iterable of files to be passed in?

[rela589n]

@greg0ire , hi! regarding iterable files approach, do you expect finder to be passed in as $paths?

That's how ORM driver uses it:

class AttributeDriver implements MappingDriver
{
    use ColocatedMappingDriver;

    public function __construct(array $paths, bool $reportFieldsWhereDeclared = true)
    {
// @@
        $this->addPaths($paths);
    }

}

[rela589n]

@greg0ire , how do you expect iterable to be given?

[greg0ire]

I would expect it to be passed in the constructor of the driver, as I don't think it should be changed afterwards. I don't think you want to add more paths at runtime, everything should be provided when compiling the dependency injection container (in the context of Symfony) IMO.

[rela589n]

So, as I understand, we should:

1. change protected array $paths (string[]) into protected iterable $files (SplFileInfo[]);
2. delete addPaths(array $paths) method as it expects array $paths, which is not comaptible with iterable;
3. change the implementation of the class by dropping RegexIterator and using an iterable $files instead.

Obviously, this is full of BC breaks. Is it acceptable?

everything should be provided when compiling the dependency injection container (in the context of Symfony) IMO

Yes, I agree with that. I'm all in for immutability when possible. The question is whether it's possible.

[greg0ire]

Obviously, this is full of BC breaks. Is it acceptable?

No.

I think you could maintain 2 separate properties and deprecate one of the 2, and throw an exception when the user attempts to write to both.

The question is whether it's possible.

I hope… if it is not, then we might allow the contribution of a method that allows to add more paths afterwards, but first, let's try without, shall we?

[rela589n]

Right now doctrine/orm driver constructor method signature looks like this:

public function __construct(array $paths, bool $reportFieldsWhereDeclared = true)

We could probably change array $paths to iterable $files, and check whether it's an array of strings that is given to keep the previous behavior.

Thus, if $files = [dir1, dir2], or even [], then previous behavior should be used. Otherwise, iterable approach kicks in.

On addPaths(), I suppose we could check if $files is initialized, and if so, then throw the exception.

So $paths property is deprecated, and should be removed 5.x if I understand correctly.

[greg0ire]

Ah, I didn't have the fact that the constructor was defined in the driver in mind. I think we should rather make this opt-in, with a boolean as a third argument to this, that can throw an exception in 4.x, just like reportFieldsWhereDeclared throws right now. That way, it will be easier to write unit tests for this, and people who want to provide a list of , say 2 strings can still do so without having to wait for 4.x.

[rela589n]

boolean as a third argument to this

Will it be any better than the suggested approach?

I presume it's possible that you didn't fully get what I intended to communicate.

It's possible to keep the existing signature (parameter-count-wise) and trigger deprecation only in cases where an old array<string> is given.

   /**
-   * @param array<string> $paths
-   * @param true          $reportFieldsWhereDeclared no-op, to be removed in 4.0
+   * @param array<string>|iterable<\SplFileInfo> $paths
+   * @param true                                 $reportFieldsWhereDeclared no-op, to be removed in 4.0
    */
-  public function __construct(array $paths, bool $reportFieldsWhereDeclared = true)
+  public function __construct(iterable $paths, bool $reportFieldsWhereDeclared = true)
   {
       if (! $reportFieldsWhereDeclared) {
           throw new InvalidArgumentException(sprintf(
@@ -48,7 +48,17 @@
       }
 
       $this->reader = new AttributeReader();
-      $this->addPaths($paths);
+
+      $isArrayOfStrings = static fn (array $items): bool => [] === array_filter(
+          $items,
+          static fn(mixed $item): bool => !is_string($item),
+      );
+
+      if (is_array($paths) && $isArrayOfStrings($paths)) {
+          $this->addPaths($paths); // this will trigger deprecation
+      } else {
+          $this->filesIterator = $paths;
+      }
   }

Later on, only iterable<\SplFileInfo> will be allowed to pass.

say 2 strings can still do so

It's possible, just as it was before

[greg0ire]

Pretty sure I understood what you said.

say 2 strings can still do so

It's possible, just as it was before

OK, I misspoke, I mean 2 filenames, when previously, strings used to mean directories. There needs to be a way to tell that the strings in the array are filenames, and should be treated as such.

[rela589n]

Why should we allow passing the file names?

[greg0ire]

Well @derrabus 's suggestion is to pass an iterable of files, right?

So we should have a constructor that allows to either pass paths (legacy way), or filenames (new way). Whether the filenames are passed with an array or a Traversable shouldn't matter, and the user should have a way to indicate that they are passing filenames and not paths, hence the need for an extra boolean.

[rela589n]

Well @derrabus 's suggestion is to pass an iterable of files, right?

Yes, that's right:

If we want to open this up for more complex cases, we could allow to pass an iterable of files, so people could use Symfony Finder etc. to crawl for files.

From the docs:

The $file variable is an instance of SplFileInfo

Thus, to use Symfony Finder, it'd be necessary first to getPathname on all the results, and then pass them as an array of file names.

From the simplicity standpoint, it's easier to pass an iterable of file names rather than the iterable of files.

[greg0ire]

Thus, to use Symfony Finder, it'd be necessary first to getPathname on all the results, and then pass them as an array of file names.

No, you could still write an iterator that calls getPathName() on an inner Symfony iterator.

[rela589n]

All right, what about excludePaths? Should it be deprecated?

[greg0ire]

Ultimately yes, but I think maybe the deprecations could be contributed later, once the Symfony Bundles (ORM/ODM) have adapted, so that people do not get unfixable deprecations. I imagine that the plan is to have all bundles instantiate a finder, and pass it configuration specified by the user.

[rela589n]

I've opened the above PR that implements it. When you get a chance, I'd appreciate it if you could review it.

[GromNaN]

Closing after revert of file paths https://github.com/doctrine/persistence/pull/431.

This should be implemented using a file paths iterator, like Symfony Finder.