Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[FrameworkBundle] Allow default action in configuration#57653
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:7.4
Are you sure you want to change the base?
[FrameworkBundle] Allow default action in configuration#57653
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We need to check if the HtmlSanitizer component is present in 7.2+ or issue a meaningful error otherwise.
@xabbuh : done. However I don't know how to update the test in FrameworkExtensionTest to try with sanitizer <7.2 & >=7.2. |
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
I don't know where and how to fix those errors (related to xml structure). If anyone can point me in the right direction 🙏 |
@Neirda24 In the |
| @@ -2382,6 +2383,10 @@ private function addHtmlSanitizerSection(ArrayNodeDefinition $rootNode, callable | |||
| ->fixXmlConfig('with_attribute_sanitizer') | |||
| ->fixXmlConfig('without_attribute_sanitizer') | |||
| ->children() | |||
| ->enumNode('default_action') | |||
| ->info('Defines how the sanitizer must behave by default.') | |||
| ->values(array_map(static fn (HtmlSanitizerAction $action): string => $action->value, HtmlSanitizerAction::cases())) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| ->values(array_map(staticfn (HtmlSanitizerAction$action):string =>$action->value, HtmlSanitizerAction::cases())) | |
| ->values(array_column(HtmlSanitizerAction::cases(),'value')) |
nicolas-grekasJul 10, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
but this won't work if the component is not installed, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes good point. Just came here to say#57686 might allow this to be cleaner, but indeed if the enum class is missing what do we do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ok so now that#57686 is merged, I would say this could probably be changed to:
->enumNode('default_action')->enumFqcn(HtmlSanitizerAction::class)->end()
The question of what to do when!class_exists(HtmlSanitizerAction::class).. I would say make the enum node conditional, and only declare it if the class exists, so that if html-sanitizer is outdated and you try to configure a default_action the config will error with an unknown option default_action. For better DX we could also if the enum class is not there define it as a dummy null value but throw with a message about html-sanitizer bieng outdated if any value is passed in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
we might not need to make the enum backed in the end, since we cannot use it to generate the XSD nor the config
| @@ -3006,6 +3007,17 @@ private function registerHtmlSanitizerConfiguration(array $config, ContainerBuil | |||
| $def = $container->register($configId, HtmlSanitizerConfig::class); | |||
| // Base | |||
| if ($sanitizerConfig['default_action'] ?? false) { | |||
| if (!class_exists(HtmlSanitizerAction::class)) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this is dead code because of the way the config is done at the moment: if there is a value, it must be one of the allowed cases, which are derived from the enum
| @@ -918,6 +918,7 @@ | |||
| <xsd:attribute name="allow-relative-links" type="xsd:boolean" /> | |||
| <xsd:attribute name="allow-relative-medias" type="xsd:boolean" /> | |||
| <xsd:attribute name="max-input-length" type="xsd:positiveInteger" /> | |||
| <xsd:attribute name="default-action" type="xsd:string" /> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
can't we restrict the possible values to the ones allowed by the enum?
What's the status of this PR? |
Uh oh!
There was an error while loading.Please reload this page.
Seesymfony/symfony-docs#20019 for documentation.