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

[Routing] Validate "namespace" (when using Psr4DirectoryLoader) #59189

Merged

Conversation

Kocal
Copy link
Member

@Kocal Kocal commented Dec 11, 2024

Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? no
Issues Fix #...
License MIT

I spend a small amount of time to understand why my controller was inaccessible:

image

With the following configuration:

controllers:
    resource:
        path: ../src/Application/Controller/
        namespace: App\Application/Controller
    type: attribute

You may have seen that the namespace App\Application/Controller is invalid because the presence of / instead of \. If I correct it, then my controller is accessible.

It means that invalid namespaces are simply ignored without any hints for the user, and I think it can be improved and be more user-friendly :)

This PR add namespace validation, it checks if / is found, and if namespace is composed of valid PHP identifiers:
image
image

@carsonbot carsonbot added this to the 7.3 milestone Dec 11, 2024
@Kocal Kocal force-pushed the feat-routing-psr4-loader-validate-namespace branch from fe08401 to b7d77a7 Compare December 11, 2024 23:11
@Kocal
Copy link
Member Author

Kocal commented Dec 11, 2024

Failing checks are unrelated, they should be fixed by #59182:
image

@@ -43,6 +43,8 @@ public function load(mixed $resource, ?string $type = null): ?RouteCollection
return new RouteCollection();
}

$this->validateNamespace($resource['namespace']);
Copy link
Member

@nicolas-grekas nicolas-grekas Dec 12, 2024

Choose a reason for hiding this comment

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

borrowed from YamlFileLoader in DI:

if (!preg_match('/^(?:[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*+\\\\)++$/', $resource['namespace'])) {
            throw new InvalidArgumentException(\sprintf('Namespace is not a valid PSR-4 prefix: "%s".', resource['namespace']));
}

Copy link
Member Author

@Kocal Kocal Dec 12, 2024

Choose a reason for hiding this comment

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

Unfortunately, this modification causes the \ trimming tests to fail, whatever if I use trim($resource['namespace'], '\\') or $resource['namespace']:

image

I will debug a bit

Copy link
Member Author

@Kocal Kocal Dec 12, 2024

Choose a reason for hiding this comment

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

It works with preg_match('/^(?:[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*+\\\)++$/', trim($resource['namespace'], '\\'). '\\'), note the original \\\\ became \\\

@Kocal Kocal force-pushed the feat-routing-psr4-loader-validate-namespace branch 3 times, most recently from 204071e to c96f1c5 Compare December 12, 2024 12:39
@Kocal
Copy link
Member Author

Kocal commented Dec 12, 2024

Went for a more simple solution.

Status: Needs review

Failing checks are unrelated:
image

@Kocal Kocal force-pushed the feat-routing-psr4-loader-validate-namespace branch from c96f1c5 to 3d807c1 Compare December 15, 2024 20:28
@Kocal
Copy link
Member Author

Kocal commented Dec 15, 2024

Status: Needs review

@fabpot
Copy link
Member

fabpot commented Dec 16, 2024

Thank you @Kocal.

@fabpot fabpot merged commit 30b8038 into symfony:7.3 Dec 16, 2024
11 checks passed
@Kocal Kocal deleted the feat-routing-psr4-loader-validate-namespace branch December 16, 2024 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants