Skip to content

[Config][FrameworkBundle] Generate JSON schema for config #59620

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

Open
wants to merge 1 commit into
base: 7.3
Choose a base branch
from

Conversation

valtzu
Copy link
Contributor

@valtzu valtzu commented Jan 26, 2025

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

Generate JSON schema in %kernel.build_dir%/config.schema.json for YAML config when the container is built to improve DX.

To discuss:

  1. Is there any way to deal with PHP enums (!php/enum yaml tag)? At the moment they are just ignored/excluded // just allowed a string !php/enum ...
  2. Should we include # $schema: path/to/schema.json in generated yaml files? probably no / up to Flex etc
  3. Any edge-cases I missed?
  4. Should we rather bump symfony/config version constraint in symfony/framework-bundle than check for class existence? This is kinda optional stuff so I thought we don't need to enforce it. already bumped in [FrameworkBundle] bump symfony/config requirement to 7.3 #59649

@GromNaN
Copy link
Member

GromNaN commented Jan 26, 2025

Thank you very much for working on this feature.
I gave it a try and it works really well.

  • The per-environment configuration need to be added. This is the same as the current root definition (that need to be a type), with key following this pattern: when@[a-zA-Z0-9]+
  • The JSON schema file will be different for each environment, because the bundles are not the sames. The JSON schema must allow any additional root keys for when@ other environments.

Here is the files that are not validated by the generated JSON schema.

doctrine.yaml

  • Some doctrine configuration are not recognized.
image

framework.yaml

routing.yaml

  • "Missing required property 'resource'. Since the configurations are merged from multiple files (and defaults) properties cannot be required in a single Yaml file.

validator.yaml

@valtzu
Copy link
Contributor Author

valtzu commented Jan 26, 2025

Thanks for quick review/comment @GromNaN!

Resolved when@ (though it does not account for env-specific extensions yet), removed "required": [...] from the schema, added using equivalentValues.

The validator issue still on to-do list, not sure on what basis to allow the empty array – maybe useAttributeAsKey just added maxItems: 0 to non-prototyped arrays.

The doctrine issue seems to be caused by normalization in a closure, some possible solutions below 👇

  1. always allow extra keys / have "additionalProperties": true in the schema (for every subschema), not to cause false-positives on keys that for whatever reason don't appear in the schema
  2. do some manual override / manually defined schema to cover this special case.
  3. convert (dynamically?) the xml schema to json schema (not sure if it matches yaml one-to-one)

@GromNaN
Copy link
Member

GromNaN commented Jan 27, 2025

Resolved when@ (though it does not account for env-specific extensions yet), removed "required": [...] from the schema, added using equivalentValues.

Thanks, perfect.

The validator issue still on to-do list, not sure on what basis to allow the empty array – maybe useAttributeAsKey 🤔

I think the JSON schema can be a bit more strict than the config validation rules. But this error may be hiding something else.

The doctrine issue seems to be caused by normalization in a closure, some possible solutions below 👇

  1. always allow extra keys / have "additionalProperties": true in the schema (for every subschema), not to cause false-positives on keys that for whatever reason don't appear in the schema

Arbitrary additional properties are not allowed by the Config component. This would fail the purpose of the JSON schema.

  1. do some manual override / manually defined schema to cover this special case.

It's the last option if you really can't do any better.

  1. convert (dynamically?) the xml schema to json schema (not sure if it matches yaml one-to-one)

I'd prefer to be able to generate the xsd rather than use it.

Maybe we don't need to support this for a first version, since there's an alternative syntax (more verbose).

@valtzu valtzu force-pushed the dx-config-schema branch 2 times, most recently from 84f3965 to 008ee37 Compare January 27, 2025 22:23
Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

I did a quick code review.

@valtzu valtzu force-pushed the dx-config-schema branch 3 times, most recently from 3cfc28e to de09b68 Compare January 29, 2025 18:55
@valtzu
Copy link
Contributor Author

valtzu commented Jan 29, 2025

Did a whole lot of cleanup based on the comments.

I also managed to workaround the Doctrine issue by allowing any type when $node->getNormalizedTypes() contains ExprBuilder::TYPE_ANY.

This of course means we don't get a schema for those, but I think it's better than false-positives when using example configuration.

Also added a deprecation message to the schema which shows up quite nicely now

image

@GromNaN
Copy link
Member

GromNaN commented Jan 29, 2025

Awesome, the deprecation message is exactly why I want such schema validation.

@valtzu
Copy link
Contributor Author

valtzu commented Feb 3, 2025

I'm a little stuck with the support for env-specific extensions: since the schema is generated inside the kernel, it does not seem possible to generate schemas for other environments. And since the kernel class is not really exposed anywhere, it's not trivial to create multiple instances of the kernel with different envs (not that it would be a great idea anyway).

Best I could think of is

  1. always only build schema for the current environment
  2. in addition to saving the schema into single file inside env-specific directory, also insert a reference into common file outside env-specific dir

This could look something like

var/cache/dev/config.schema.json
var/cache/test/config.schema.json
var/cache/whatever/config.schema.json
var/config.schema.json # references all files above, like {"when@dev": { "$ref": "cache/dev/config.schema.json" } }

The downside with this solution is that unless you build the container for those other envs, you also don't get the schema for those. Also, I doubt that many people build the container locally for any env but dev & test.


If someone has a better approach, great!

Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

We could look at bundles.php to find all the environments and bundles used, but that wouldn't cover generic use cases (if bundles are registered directly in Kernel.php, for example).

We could also scan all the directories and find all the classes that implement the BundleInterface, but that would require scanning composer autoloader and would take up too many bundles.

I think the idea of generating independent schema files for each env is more realist:

  • generate var/cache/dev/config.schema.json, var/cache/test/config.schema.json and var/cache/prod/config.schema.json.
  • when a file is generated, create or update the parent var/cache/config.schema.json to include the generated fields.

I tried this with PHPStorm, but the file loading doesn't work yet.

{
  "$schema": "http://json-schema.org/draft-06/schema#",
  "allOf": [
    { "$ref": "./dev/config.schema.json" },
    { "$ref": "./test/config.schema.json" }
  ]
}

$allowNull = $allowNull || \in_array(ExprBuilder::TYPE_NULL, $normalizedTypes);
if (\in_array(ExprBuilder::TYPE_ANY, $normalizedTypes)) {
// This will make IDEs not complain about configurations containing complex beforeNormalization logic
$subSchemas[] = new \ArrayObject();
Copy link
Member

Choose a reason for hiding this comment

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

For an empty object, it's more correct to use stdClass. You are using the fact that any object that doesn't expose any property is encoded as an empty object.

Suggested change
$subSchemas[] = new \ArrayObject();
$subSchemas[] = new \stdClass();

Copy link
Contributor Author

@valtzu valtzu Feb 18, 2025

Choose a reason for hiding this comment

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

I'm intentionally using \ArrayObject (just like Symfony Serializer does internally) so that I can perform array operations on it, f.e. $schema['default'] = $node->getDefaultValue() and \count($subSchema, \COUNT_RECURSIVE).

If \stdClass was used instead, I think the logic would be overall more complex.

Switched to \stdClass everywhere

}
}

return $subSchemas ?: [new \ArrayObject()];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return $subSchemas ?: [new \ArrayObject()];
return $subSchemas ?: [new \stdClass()];

$schema['type'][] = $type;
}

private function getReference(array|\ArrayObject $subSchema, array &$definitions): array|\ArrayObject
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private function getReference(array|\ArrayObject $subSchema, array &$definitions): array|\ArrayObject
private function getReference(array|\stdClass $subSchema, array &$definitions): array|\stdClass

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using \stdClass for schemas everywhere now & thus got rid of array by-ref, reasoning in #59620 (comment)


public function isOptional(): bool
{
return false;
Copy link
Member

Choose a reason for hiding this comment

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

I think this is optional. The app doesn't relies on it to run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@valtzu valtzu force-pushed the dx-config-schema branch 2 times, most recently from 8715b5d to 003b6f9 Compare February 18, 2025 21:16
@valtzu
Copy link
Contributor Author

valtzu commented Feb 18, 2025

I tried this with PHPStorm, but the file loading doesn't work yet.

Yeah, that is sad. I ended up putting everything in a single file, %kernel.build_dir%/../config.schema.json.

Also switched from array to \stdClass because after merging configs between envs I was getting [] where {} was expected, breaking the whole schema.

So now you'd need to run

bin/console --env=dev
bin/console --env=test
bin/console --env=prod

etc to populate all the configs – though in reality I think most projects have all the bundles & extensions available in dev, thus nothing extra except dev build should be needed.

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.

[DX][Config] Generate JSON schema for Yaml configuration
3 participants