Skip to content
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

fix glob pattern for resource #286

Closed
wants to merge 1 commit into from

Conversation

franzholz
Copy link
Contributor

All files in all folders must be found by the Glob Pattern.
The current pattern will only find the files in the Classes folder.

All files in all folders must be found by the Glob Pattern.
@garvinhicking
Copy link
Contributor

Thanks for the patch! However: I don't think so, all TYPO3 core sysexts only use this 'Classes/*' pattern, not a single one uses '**'.

I think the reason is that composer receives a list of folders, and it dives any folder it gets. So "**" (any file in any folder) is not needed (and maybe even problematic?)

@franzholz
Copy link
Contributor Author

Uh, I cannot read as fast as you close it.

So you think that the files under the subfolders are not needed? Why aren't they needed also?

@garvinhicking
Copy link
Contributor

They are needed - Composer loads all of them. The glob pattern returns a list of directories, not files for this YAML directive :)

@garvinhicking
Copy link
Contributor

Correction: Symfony DI seems to do the directory-based resource importing (it's not file based)

@franzholz
Copy link
Contributor Author

The resources glob pattern from Services.yaml has nothing to do with Composer. These files are fetched into your system by Composer. But the PHP processing is done by Symfony. And Symfony uses the glob pattern. '' does not search all folders inside of the subfolders. Only '**' does this.
'
' does not match slashes. Therefore the files in folders of the subfolders are not found.

https://stackoverflow.com/questions/55554147/meaning-of-and-other-symbols-in-a-symfony-glob-pattern

https://www.malikbrowne.com/blog/a-beginners-guide-glob-patterns/

The wildcard * stands for "any string of any length including empty, but excluding the path separator characters (/ in unix and \ in windows)".

This means for the example extension that the files inside of /Classes/LinkValidator/LinkType are not matched!

@garvinhicking
Copy link
Contributor

It works with the definition in the Core, so IMO no further discussion necessary on this :)

@franzholz
Copy link
Contributor Author

This is no valid argument: "it works for me!".

@garvinhicking
Copy link
Contributor

Ok, then the other way: Please explain why it works for every TYPO3-core extension, even though it does not list "../Classes/**". If your statement would be true, it shouldn't work. We cannot change documentation and divert from what the TYPO3 core itself uses.

@franzholz
Copy link
Contributor Author

I have checked the info system extension.
Here you have the wrong Services.yaml entry:

 TYPO3\CMS\Info\:
    resource: '../Classes/*'

Why it still works? Just because no Dependendy Injection is used for the classes in the subfolders below the first level folders.

typo3_src-12.4.16/typo3/sysext/info/Classes/Controller/InfoModuleController.php:

        $event = $this->eventDispatcher->dispatch(
        new ModifyInfoModuleContentEvent($accessContent, $request, $this->currentModule, $this->view)
    );

The construction is passing all arguments by itself. It does not use the Dependency Injecion.

typo3_src-12.4.16/typo3/sysext/info/Classes/Controller/Event/ModifyInfoModuleContentEvent.php:

final class ModifyInfoModuleContentEvent
{
    private string $headerContent = '';
    private string $footerContent = '';

    public function __construct(
        private readonly bool $access,
        private readonly ServerRequestInterface $request,
        private readonly ModuleInterface $currentModule,
        private readonly ModuleTemplate $moduleTemplate,
    ) {}

@garvinhicking: "We cannot change documentation and divert from what the TYPO3 core itself uses."

Why? TYPO3 should never be programmed based on other search patterns than they are commonly used in all the IT world outside of TYPO3!

@sbuerk
Copy link
Contributor

sbuerk commented Aug 28, 2024

The discussion is a useless one. Literally, it does not matter if you use *, ** or none at all. Symfony DI uses the folder as startingpoint and reconizes ALL subfolders as services to be determined if they need autowiring, injection or similar stuff.

From the Symfony DI Documentation:

image

    # makes classes in src/ available to be used as services
    # this creates a service per class whose id is the fully-qualified class name
    App\:
        resource: '../src/'
        exclude:
            - '../src/DependencyInjection/'
            - '../src/Entity/'
            - '../src/Kernel.php'

Symfony DI uses the Symfony Finder component which does not follow glob as hard as you may expect. Using single * is enough and supported here.

Literally, the single wildcard is supeflous BUT it does work. Otherwise absolutly NO DI would work in TYPO3 which is literally NOT the case.

I would suggest to keep the variant the core uses and eventually refer to the Symfony DI documentation for advanced usages WHICH should already be the case. We do not have to mirror the Symfony DI documentation here and just gave the basic step into it.

@franzholz You picked an Event as example. An event is a DTO (Data Transfer Object) and does per definition not a service. DI is only for services. So it is correct that it does not use DI, and this is by design to follow design principles.

Maybe you ask your specific "not working" question in the slack cms channel first. I guess your problem lies on something else than that extension resource namespace configuration.

EXT:core Services.yaml

  TYPO3\CMS\Core\:
    resource: '../Classes/*'

Following your definition NO DI should work throughout the core and other extension letting core classes be injected. Which is the case in a multitude of places.

As explined aboute, the * is superflous in the used form but than we should change that in the core first and change the example in the documentation to avoid the wildcard at all.

I can only guess that maybeyour classes namespaces does not match the composer autoload PSR-4 pattern or follow it in the subfolder (missing the subfolder in the phpnamespace etc).

@sbuerk
Copy link
Contributor

sbuerk commented Aug 28, 2024

All files in all folders must be found by the Glob Pattern.
The current pattern will only find the files in the Classes folder.

Please bake your "assumption" / statement with evidence. It's litterally wrong.

@sbuerk
Copy link
Contributor

sbuerk commented Aug 28, 2024

Read up Symfony Finder documentation, it uses single asterix as patterns even for folders:

https://symfony.com/doc/current/components/finder.html

Use * as a wildcard character to search in the directories matching a pattern (each pattern has to resolve to at least one directory path):

 Copy
$finder->in('src/Symfony/*/*/Resources');

And as Smyfony DI uses the finder - the same rule applies here. It does not use glob() method of PHP/system call.

@franzholz
Copy link
Contributor Author

All files in all folders must be found by the Glob Pattern.
The current pattern will only find the files in the Classes folder.

Please bake your "assumption" / statement with evidence. It's litterally wrong.

This is not an assumption about the Glob Pattern. Check the Glob Pattern syntax and you see that this is the reality.

But sorry, I did follow the wrong assumption of a Glob Pattern being used for files. I have not seen any documenation about this neither in TYPO3 nor in Symfony.

@franzholz
Copy link
Contributor Author

franzholz commented Aug 28, 2024

Use *

I do not get what you mean!
Please explain it!

Jesus - read the Symfony documentation. That was copy & pasted from the Symfony documentation of the Symfony Finder.

And it is the explanation why the single asterix in the Services.yaml is correct. Finder uses single asterix and not the glob double asterix pattern.

The point is that you forces to apply glob rules which does not uses glob at all.

@sbuerk
Copy link
Contributor

sbuerk commented Aug 28, 2024

All files in all folders must be found by the Glob Pattern.
The current pattern will only find the files in the Classes folder.

Please bake your "assumption" / statement with evidence. It's litterally wrong.

This is not an assumption about the Glob Pattern. Check the Glob Pattern syntax and you see that this is the reality.

But sorry, I did follow the wrong assumption of a Glob Pattern being used for files. I have not seen any documenation about this neither in TYPO3 nor in Symfony.

It was your state ment that all files needs to be found by the glob pattern. Symfony DI(Finder) does not use glob thus this statement from you is wrong.

Using * is correct and works.

This is not an assumption about the Glob Pattern. Check the Glob Pattern syntax and you see that this is the reality.

Again - you have a wrong assumption here. It does not need to follow glob pattern because the fact is that glob is not used. The Symfony Finder is used within Symfony DI it uses single asterix as pattern placeholder and not the double asterix. Point.

@franzholz
Copy link
Contributor Author

And as Smyfony DI uses the finder - the same rule applies here. It does not use glob() method of PHP/system call.

??? But Symfony uses the Glob pattern. This can be investigated from the PHP Code.

https://github.com/symfony/finder/blob/7.1/Glob.php

@franzholz
Copy link
Contributor Author

Here again that you can see it, that Symfony finder uses the Glob pattern.

https://symfony.com/doc/current/components/finder.html

[File Name](https://symfony.com/doc/current/components/finder.html#file-name)
Find files by name with the [name()](https://github.com/symfony/symfony/blob/7.1/src/Symfony/Component/Finder/Finder.php#:~:text=function%20name) method:

 Copy
$finder->files()->name('*.php');
The name() method accepts globs, strings, regexes or an array of globs, strings or regexes:

 Copy
$finder->files()->name('/\.php$/');

@sbuerk
Copy link
Contributor

sbuerk commented Aug 28, 2024

And as Smyfony DI uses the finder - the same rule applies here. It does not use glob() method of PHP/system call.

??? But Symfony uses the Glob pattern. This can be investigated from the PHP Code.

https://github.com/symfony/finder/blob/7.1/Glob.php

Nope. The use a OWN glob implementation and not the native php glob(). Different things.

Again. The syntax is correct. End of discussion.

@sbuerk
Copy link
Contributor

sbuerk commented Aug 28, 2024

Here again that you can see it, that Symfony finder uses the Glob pattern.

https://symfony.com/doc/current/components/finder.html

[File Name](https://symfony.com/doc/current/components/finder.html#file-name)
Find files by name with the [name()](https://github.com/symfony/symfony/blob/7.1/src/Symfony/Component/Finder/Finder.php#:~:text=function%20name) method:

 Copy
$finder->files()->name('*.php');
The name() method accepts globs, strings, regexes or an array of globs, strings or regexes:

 Copy
$finder->files()->name('/\.php$/');

Read above - I pasted you the FINDER example for asterix placeholder (pattern) example using the finder compontent right from the symfony documentation.

Maybe you want to fight first with symfony to change their documentation away from how it works to what you mean it should be documented ?

@sbuerk
Copy link
Contributor

sbuerk commented Aug 28, 2024

The documented pattern is correct and the pattern we want to document as best practice. If you have special needs change it in your extension which is absolutly fine.

Just stay away that it does not work (in core). Beside the core a lot of projects and extensions uses this pattern quite fine even for classes in subfolders.

@franzholz
Copy link
Contributor Author

Maybe you want to fight first with symfony to change their documentation away from how it works to what you mean it should be documented ?

This is nonsense. I have shown the documentation and Symfony PHP code which contradicts your wrong sentence about the Symfony Finder not using Glob patterns. And now you still claim it again and argue that I wanted to fight!

@mabolek
Copy link
Contributor

mabolek commented Aug 28, 2024

Dear all, please remember to keep a nice and respectful tone and adhere to TYPO3's Code of Conduct. 🙂

@franzholz
Copy link
Contributor Author

see #288

@franzholz
Copy link
Contributor Author

Just for the completeness I add this link. There have been more wrong things in the previous discussion about how Symfony works with the glob pattern in Services.yaml.

symfony/symfony#35679

the most common path uses only native glob() function. Symfony\Component\Config\ResourceGlobResource which is used inside symfony/dependency-injection begins with native glob() function, then extended version from Symfony\Component\FinderGlob::toRegex.

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.

4 participants