Implement file regex search for ColocatedMappingDriver

[rela589n]

Hello,

This PR adds a flexibility for ColocatedMappingDriver, allowing client code to customize files search by specifying an regex (rather than just file extension as previously).

This is very useful for the cases when tests, fixtures, factories are kept in the same namespace as entity (having one namespace for one Aggregate).

For example, if we structure classes for User Aggregate:

User/
├── User.php
├── UserFixture.php
└── Actions/
    └── Register/
        ├── RegisterUserCommand.php
        └── RegisterUserTest.php

Like here we have User/ namespace that includes everything possessed by the concept of User (e.g. Entity, Fixture, Command, Test, Controller, maybe some Value-Objects, etc.)

Right now Doctrine mapping is not ready for this kind of architecture, as it would scan all these files and include them, while they don't have to be included, so I would like to fix it with regex. It would not have been a big problem with all these files being included if there were no dev-things there, but they are there, so it will possibly break in prod environment, if we keep them.

Therefore, I introduced new $fileRegex property that is used when filtering, and it could be set from the client code.
Old $fileExtension I have deprecated so that we could use $fileRegex.

Please, let me know if you don't have objections about this, and what further steps I need to do?

Also, please note that this is my first PR to Doctrine Project, so I would appreciate it if you'll be patient with me.

[greg0ire]

Hi! I retargeted on 4.1.x since this is no bugfix, but now there are conflicts, please address them.

[rela589n]

@greg0ire , fixed the conflicts

[greg0ire]

Thanks for your contribution. Which documentations will need to be upgraded if we go with this change?

Also, you should add upgrade instructions in UPGRADE.md

[greg0ire]

I think it would be fair to write more tests targeting this change beside testing getters and setters.

[rela589n]

Actually, the resulting regex has never changed.

So, as it was '/^.+\.php$/i', so it is now:

self::assertSame('/^.+\.php$/i', $driver->getFileRegex());

I've added this assertion to testSetFileRegex that verifies that regex is actually one we expect same as was before.

[greg0ire]

And that's great, definitely something we'd want to test. But I think the new functionality you unlocked should be demonstrated in a test based on the filetree you showed in the description of this PR.

[rela589n]

do you want me to create a new test for it, or maybe I could adjust testGetAllClassNames() so that it would use regex to find only needed classes?

[stof]

I think it should be a new test

[rela589n]

All right, so I added the test that will collect only needed entities by regex: testGetAllClassNamesWithRegex

I think it should be quite obvious that if testGetAllClassNames asserts about [Entity::class, EntityFixture::class], while new test asserts only about [Entity::class], this means that fixture has been excluded by regex.

[rela589n]

Hi, @stof , @greg0ire , could you check it once more please?

[greg0ire]

Which documentations will need to be upgraded if we go with this change?

Please answer this.

[rela589n]

Which documentations will need to be upgraded if we go with this change?

Please answer this.

Sorry, I didn't get the point. What documentations do you mean? I thought you were talking about UPGRADE.md, or is it anything else you are talking about?

[greg0ire]

I mean: is there a documentation page on doctrine-project.org or maybe even symfony.com detailing how to configure mapping drivers? In the end, how will end-users know how to configure their projects to allow using the file tree you described in your original post?

[rela589n]

I see that ColocatedMappingDriver is only used in AttributeDriver, yet setFileExtension is never used.

Doctrine Bundle doesn't set it, as it doesn't have support for it.

Also, I searched a bit through the docs, and see that method setFileExtension is all about other places, not related to ColocatedMappingDriver:

• \Doctrine\Common\ClassLoader (common)
• \Doctrine\RST\Configuration (rst)
• \Doctrine\ORM\Mapping\Driver\XmlDriver (and SimplifiedXmlDriver) (orm xml driver) - this one is outdated, since there's no such method as setFileExtension for XmlDriver, as it's not in abstract FileDriver. Right now it seems to be possible with setLocator, or ->getLocator()->setFileExtension() (yet this isn't in FileLocator interface), so I think this doc should be updated.

Regarding the new setFileRegex, I think it could be referenced in advanced orm configuration options.

[rela589n]

@greg0ire , hi, could you please guide on the next steps to take?

[greg0ire]

No occurrences in the ODM docs? I think next steps would be to get more approvals on this, so I'll request more reviews. In parallel to that, you could open draft PRs updating the docs, it would help reviewers understand the changes you are proposing, and will be needed anyway.

[rela589n]

No occurrences in the ODM docs?

I checked for ODM docs, and found one more entry (relevant to setFileExtension, but not to ColocatedMappingDriver):

• Doctrine\ODM\MongoDB\Mapping\Driver\XmlDriver (and SimplifiedXmlDriver) (odm xml driver) - this doc seems to be outdated, since there's no such method as setFileExtension in XmlDriver, as it's neither in abstract FileDriver. This is the same very issue described in my last comment, so I think this doc should be updated as well.

No different from already described.

Also, there are few references from Yaml mapping drivers (odm, orm), though I don't think they should be updated, since they've been removed.

In parallel to that, you could open draft PRs updating the docs

I'll try to do it soon.

[rela589n]

Hi guys, hope you are doing well!

@greg0ire , @SenseException , could you suggest moving forward

[greg0ire]

This is going in circles, let's stop the conversation here if you don't mind.

[rela589n]

@greg0ire , actually I do mind

[rela589n]

I've clearly stated my vision, and this's no reason to close the PR

[rela589n]

@greg0ire , I really believe that this should've been implemented this way

[greg0ire]

Well what's the point of reopening it if no one is going to merge it? I certainly am not…

I really believe that this should've been implemented this way

I already got that, and already told you I got that, and I disagree, and it's not just me, so no need to reiterate, really.

[rela589n]

Please, let me explain myself.
I'm reaching for a better quality code software.
I stumbled upon the problem that tests were included in non-dev environments.
Evidently, I went and searched how it migth be customized, and finally I found that it was almost there. Almost what I needed. Quite nearly, almost totally, but not.
And if there was a setter for a regex rather than setter for an extension, it would be it.

[greg0ire]

I understood that as well. What you don't seem to understand is that opening this can of worms opens up this repository to a lot of support requests from users who won't know how to use this properly (because let's face it, it's hard to use). In other words, it changes the public API of this library from straightforward to quite complex. What @derrabus pointed to is another way to achieve the same goal but would be user friendly, but you don't want to implement it. That's fully OK, but don't try to get us to merge this then.

[rela589n]

can of worms

This is not a can of worms. It's no good to call any regex solution bad because some might not understand regex. You might mean bugs in the code due to regex. This feature is going to be as one time action taken during project setup. If anybody needs this, he'll do it, if not - he'll not be bothered with it. This is not something that people will need to use and know much about, as they won't be thinking about it, nor using it all the time.

a lot of support requests

Have there been support requests about regex?

changes the public API of this library from straightforward to quite complex

It doesn't change API of the library from straightforward to complex. There isn't any API (no straightforward, no other) changed. Code opens for usage that, what already was there.

another way to achieve the same goal
you don't want to implement

Yes, I do not want to implement other solution, as the implementation plan is obscure, and it requires changes in other places, and possibly other libraries, it anticipates completely different approach to the one that already exists, and it's not clear how this is supposed to work with the given paths which are already collected, and how this all is eventually expected to be integrated with symfony anyway.

It might look interesting to have the ability to use symfony finder, but I do not find it clear, and neither reasonable or accomplishable due to the current way colocated mapping driver works.

[greg0ire]

There isn't any API (no straightforward, no other) changed

Well the old API is deprecated, so it will be removed in the next major, so eventually, it's a change even if right now, it just looks like an addition.

Yes, I do not want to implement other solution, as the implementation plan is obscure,

Then clarify it. Are we the ones that have to do all the thinking to solve your issue? To help you, the plan looks like this, to me:

1. In the bundles, define configuration allowing to define inclusion and exclusion patterns (as either globs or regexes), because symfony/finder supports both.
2. Still in the bundles, use symfony/finder to build an iterable of files from the configuration nodes this.
3. Pass the iterable of files to the library.

Maybe you can get more details/validation from @derrabus since it was his idea.

[rela589n]

Well the old API is deprecated

Well, this isn't used anyway, so is looks like "straightforward useless API is changed into quite complex useful API" 😄 , though I do not find it complex at all.

Then clarify it. Are we the ones that have to do all the thinking to solve your issue?

That's your proposed solution, and it might be fine, but if it's obscure and I don't get a point, it's just not reasonable to think that "a lot of changes" will be any better than "small regex", as far as end result remains the same

[greg0ire]

though I do not find it complex at all

The new API has an argument that is a regex, which embeds a big complexity because it's basically a small state machine, and when you look at usage examples, you see that you have to know about advanced regex concepts to make it useful. Compare that to an API that takes an iterable of filepaths.

it's just not reasonable to think that "a lot of changes" will be any better than "small regex", as far as end result remains the same

The end result might be the same but the usage to reach that end result definitely isn't, as outlined in #417 (comment)

[rela589n]

I agree that the example might be complex.

Consider this instead: (?<!Fixture)\.php$ (https://regex101.com/r/YLJJ7G/1)

Basically this is the same, and excludes everything that ends with Fixture.php

[rela589n]

@greg0ire , Sorry for providing the complex example. I agree that it's nuts and crazy.

Given that the example is changed, can we proceed with this?

So, ^(?!.*(Test|Fixture|Command)\.php$).*\.php$ is replaced with (?<!Test|Fixture|Command)\.php$

[rela589n]

@greg0ire , (?<!Test|Fixture|Command)\.php$ looks simple enough, isn't it?

[greg0ire]

I would say it still looks a bit nuts and crazy.

[rela589n]

It's not.

[rela589n]

It's not nuts, nor crazy. It's the simplest thing that could be.

[greg0ire]

It's probably the simplest regex that could be for this case, I'll concede that.

[rela589n]

To tell the truth, I'm glad to see it. So let's reopen it

[rela589n]

@greg0ire , hi! So let's reopen it

[greg0ire]

Hi! I think you misunderstood, it's probably the simplest regex, and that's not good enough, sorry.

[rela589n]

@greg0ire , it's simple enough so that anybody can understand it

[rela589n]

@greg0ire , how can we move on?

[greg0ire]

By implementing the solution proposed by @derrabus

[rela589n]

@derrabus , can we elaborate on this? What is the interface that you propose?

[rela589n]

@derrabus , @greg0ire , just a reminder that I would want to move on.

[rela589n]

Hi, @derrabus !

I guess you've missed the message. Could you please reply

[rela589n]

@greg0ire , @derrabus , can you please respond?