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

Allow XML validation to be disabled but keep it enabled by default. #11165

Merged
merged 4 commits into from Jan 26, 2024

Conversation

jwage
Copy link
Member

@jwage jwage commented Jan 17, 2024

After discussing this issue, we decided to allow XML validation to be disabled for now until we have a proper solution to allow validating against custom schemas. Disabling the XML validation is the only solution available at the moment so we want to continue allowing that until we have an alternative solution to offer.

@jwage jwage requested a review from greg0ire January 17, 2024 19:14
greg0ire
greg0ire previously approved these changes Jan 17, 2024
@greg0ire
Copy link
Member

greg0ire commented Jan 17, 2024

🤔 I'm wondering how the deprecation messages on 2.17.x need to be adjusted now. Maybe the flag should be made mandatory in this PR, and what would become deprecated would be to rely on the default false value (since it's going to change)? But with the bundle calling createXMLMetadataConfiguration, the flag is probably always provided anyway, with the bundle's default value. So maybe what's best is to remove the deprecation entirely, and let people find about the deprecations on 3.0.x the hard way.

@jwage
Copy link
Member Author

jwage commented Jan 17, 2024

@greg0ire Seems like we should just remove the deprecation from 2.17.x?

@greg0ire
Copy link
Member

Maybe yes, I wonder what other maintainers think.

@jwage
Copy link
Member Author

jwage commented Jan 19, 2024

Maybe yes, I wonder what other maintainers think.

@beberlei what do you think? Can it be as simple as just backing out the deprecation for this from 2.x?

@greg0ire
Copy link
Member

greg0ire commented Jan 19, 2024

Gave it a little more thought, and I think a good solution might be to:

  1. remove existing deprecations
  2. add a deprecation here, if func_num_args() is 1. The deprecation should warn that the argument is going to become mandatory in 3.0.0:
    $config = self::createConfiguration($isDevMode, $proxyDir, $cache);
  3. In DoctrineBundle, remove the default value for the config node, and in the DI extension, pass the argument to the service only if specified in the configuration. If not specified, maybe trigger another deprecation saying that the configuration node is going to become mandatory in the next major version of the bundle?
  4. Change this PR so that the ORMSetup argument mentioned in point 2. becomes mandatory.
  5. In the Symfony recipe, set the configuration node to true.

That way, it's fine to disable deprecations, it just has to be done explicitly.

@greg0ire greg0ire added this to the 3.0.0 milestone Jan 26, 2024
@greg0ire greg0ire merged commit 1a5942a into doctrine:3.0.x Jan 26, 2024
60 checks passed
@greg0ire
Copy link
Member

Thanks @jwage !

@jwage jwage deleted the allow-xml-validation-disable branch January 26, 2024 19:41
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.

None yet

3 participants