Allow parsing and writing PHP class constants and enums in YAML files

Can you think of a usecase in core so we have some implicit test coverage?

It seems that the PECL YAML parser supports custom callbacks, so we could easily add one for !php/const: http://php.net/manual/en/yaml.callbacks.parse.php

+++ b/core/profiles/standard/config/install/field.storage.node.field_tags.yml
@@ -12,7 +12,7 @@ settings:
-cardinality: -1
+cardinality: !php/const Drupal\Core\Field\FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED

We need an upgrade path for this.

+++ b/core/profiles/standard/config/install/field.storage.node.field_tags.yml
@@ -12,7 +12,7 @@ settings:
 locked: false
-cardinality: -1
+cardinality: !php/const Drupal\Core\Field\FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED
 translatable: true

I'm not 100% convinced about this change here. If you think about it, resaving the UI would then trigger a config diff, doesn't it? Maybe we should add a constant to a test only file.

The patch in #7 fails to apply on 8.6.1 and 8.7.x. Attached is a new patch that does.

I was not able to create an interdiff somehow, sorry for that.

Im all for this, if at core we are wrapping symfony components, these components in my opinion must have all features enanbled for them, or (probably more work) be leveraged/used in a configurable manor so that the developers can add theses options easily without patches etc.

I would say some great use cases for this are:
- route id's,
- permission keys,
- anywhere where we can swap the use of 'hard typed' strings with accessbile constants.

Thanks for the patch, #12 applied successfully on 8.6.5

Tested on drupal 9.4.5, thanks!!
Created MR to be easiest to merge.

@Eduardo Morales Alberti, thanks for the interest. Can you add what you did to test this patch?

Reviewing the comments I see that #3, #5, #9 and #10 have not been addressed. Setting to NW for those.

I am using patch #12 on a large project to store route names in class constants.
I works very well for me.

!php/const Drupal\example_dashboard\Routes::METRICS:
  path: '/dashboard/metrics'
  defaults:
    _title: 'Metrics'
    _controller: '\Drupal\example_dashboard\Controller\DashboardController'
  requirements:
    _permission: 'access to example dashboard'

@quietone okey, I will answer

#3 Can you think of a usecase in core so we have some implicit test coverage?

Maybe the usecase in core is the same that the patch. field.storage.node.field_tags.yml, this MR need tests.

It seems that the PECL YAML parser supports custom callbacks, so we could easily add one for !php/const: http://php.net/manual/en/yaml.callbacks.parse.php

The comment #6 patch do what @amateescu comments, add a custom callback:

+      '!php/const' => '\Drupal\Component\Serialization\YamlPecl::parsePhpConstant',

#9/#10
+++ b/core/profiles/standard/config/install/field.storage.node.field_tags.yml
@@ -12,7 +12,7 @@ settings:
-cardinality: -1
+cardinality: !php/const Drupal\Core\Field\FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED

We need an upgrade path for this.

Yes, we should add an upgrade for the file.

This is lovely. It'd be great to add supported to enums at the same time.

We can extend \Drupal\Tests\Component\Serialization\YamlTest::testYamlFiles() to add explicit test coverage.

Rebased the MR on to 11.x

This will allow us to use enums as container arguments which would have been lovely for https://www.drupal.org/project/drupal/issues/3402168

I'm going to change this to a task because I don't really think this is a feature request. We need this change in order to support modern PHP in the container and config system.

This is looking great already! 🤩

Our test coverage of this change is not create until #3402448: Add PECL Yaml to PHP 8.1 and PHP 8.2 is fixed.

Created https://git.drupalcode.org/issue/drupal-3386474/-/pipelines/51765 to test new dev images with YAML extension enabled

Core should be fixed before we can roll new image

Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
[33mException Other      phpunit-208.xml      0 Drupal\Tests\Core\DependencyInjecti
[0m    PHPUnit Test failed to complete; Error: PHPUnit 9.6.13 by Sebastian
    Bergmann and contributors.
    
    Testing Drupal\Tests\Core\DependencyInjection\YamlFileLoaderTest
    .............E                                                    14 / 14
    (100%)
    
    Time: 00:00.074, Memory: 6.00 MB
    
    There was 1 error:
    
    1) Drupal\Tests\Core\DependencyInjection\YamlFileLoaderTest::testExceptions
    with data set "YAML must be valid" ('   do not:\n      do this for...o
    Bar!', 'The file "vfs://drupal/module...d YAML')
    Array to string conversion
    
    /builds/issue/drupal-3386474/core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php:454
    /builds/issue/drupal-3386474/core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php:423
    /builds/issue/drupal-3386474/core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php:71
    /builds/issue/drupal-3386474/core/tests/Drupal/Tests/Core/DependencyInjection/YamlFileLoaderTest.php:78
    /builds/issue/drupal-3386474/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
    /builds/issue/drupal-3386474/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
    /builds/issue/drupal-3386474/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
    /builds/issue/drupal-3386474/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
    /builds/issue/drupal-3386474/vendor/phpunit/phpunit/src/TextUI/Command.php:144
    /builds/issue/drupal-3386474/vendor/phpunit/phpunit/src/TextUI/Command.php:97
    
    ERRORS!
    Tests: 14, Assertions: 32, Errors: 1.
[31mFail      run-tests. Unknown              0 Unknown                            
[0m    FATAL Drupal\Tests\Core\DependencyInjection\YamlFileLoaderTest: test runner
    returned a non-zero error code (2).

opened #3402548: YamlFileLoaderTest fails if you have PECL yaml installed to fix the HEAD fail with PECL yaml.

Just rebased since the issue in #38 was merged.

See it was tagged for a CR, so only moving to NW for that.

Looking through the code, I expect this is probably only reading constants not writing them unless there's some magic I missed so the title/scope should probably be updated to avoid confusion.

That makes this only useful to developers writing config directly like comments that also cannot be sync'd but that was probably always the only reasonable scope. I think that's still worthwhile for maintainability of core and contrib so +1

Our CR should probably be clear about the side effects/impact of this though. If you write a constant in your install config, then at some point in the future change that constant's value (like bumping password strengths or something) it won't change installed config on sites. You'll need to provide a method for user's to get that updated value. This is also going to be hard to test for because your test site in CI will get the new value during install masking the impact.

@neclimdul Symfony yaml can write enums - which come out as constants... see the test added here. Also core only ever writes with Symfony yaml... you are not supposed to write with PECL yaml. It is not supported since forever.

I'm aware symphony does all the writing in drupal. We don't touch the encoder though and the test asserts the parsed yaml matches the value of the constant, not that the constant format is in the encoded yaml.

I'll do some manual testing in a few days to make sure I'm not missing something though.

@neclimdul there is no value for
$this->assertSame($symfony['foo'], EnumValue::Yes); - it's not backed... this is an instance of test. It's the same for the backed enum - it would have to be BackedEnumValue::Maybe->value and not BackedEnumValue::Maybe for a comparison of values. Which would fail....

Added the CR...

In doing that I've realised that we have a problem... due to config import. We read configuration prior to doing the import. At this point, if a module that is providing a enum that's in config is being installed everything will crash. Because the enum won't exist and be autoloadable yet. Fixing that would take a complicated changes to the ConfigImporter. I think in the long run we're going to have to make because if we want to validate config we're going to need to be able to load the custom constraints... but it is all very very awkward.

Re #46 even if we fix the config importer we'd still have the config comparing via the UI and Drush... so maybe we just have to say no to enums in config - until we stop futzing with the autoloader and just let composer do it all - which would be a massive massive change.

This looks very close, but I think we need a bit more test coverage to ship this with full confidence.

@Wim Leers
1. That doesn't really matter... we're not storing the backed value FWIW.
2. That's just a regular const - no different from another class constant. It's not a special case.
3. When Symfony writes out an enum it uses php/const not php/enum - enums in PHP are really constants under the hood.

1. I know it doesn't really matter. Personally, for something as low-level and fundamental as this, I find it reassuring to see the whole spectrum explicitly tested. Especially when writing tests for it isn't painful. Anyway, no big deal, just a matter of preference 👍
2. [See point 3 first.] That may be, but I'd be interested to see how it gets serialized into YAML: as !php/enum SomeEnum::ValueUsingAConstant or as !php/const TheConstant.
3. Huh — then why does https://symfony.com/blog/new-in-symfony-6-2-improved-enum-support#enums-... literally show !php/enum SomeEnum::Foo? 😳

   Did some investigating, and you're right! 🤪

   Quoting @nicolas-grekas from https://github.com/symfony/symfony/pull/46771:

   This PR allows one to use the following Yaml together with enums:

   - !php/enum SomeEnum::Bar
   - !php/enum SomeEnum::Bar->value
   

   The first line gets the SomeEnum::Bar instance. It's the same as writing !php/const SomeEnum::Bar but with an additional check that this is really an enum.

   IOW:

   1. Symfony will only encode enums using ONLY !php/const
   2. Symfony will only decode enums using BOTH !php/const and !php/enum, and in the latter case it will run extra checks.

   I think it'd be very confusing to use Symfony's advanced YAML capabilities but then do it in a way that only a subset of what their public docs/announcements show. Even if in Drupal's configuration we cannot encode !php/enum, I think it would be valuable to support decoding it: to match what Symfony supports/its docs claim, as well as for the extra assurances.

   More importantly: otherwise the PECL and Symfony YAML decoders would behave differently on the same YAML. Because Yaml::PARSE_CONSTANT triggers support for both !php/const and !php/enum.

   Just pushed suggested additional test coverage: once that passes my concerns would be addressed. If you disagree Drupal should support Symfony's !php/enum even when using PECL, then … that's fine, I defer to you, but then at least I've expressed my concern and demonstrated it to the best of my ability 😄

Sure lets support Symfony's !php/enum this will be useful for services. As per #46 I'm not convinced that we can use this in Config :(

Right 😞 We'd need the config subsystem to special-case core.extension more than it already does. Not simple, but doable, and probably worthwhile. Once this issue is done we'll have at least one use case already — and I suspect more once we factor in Recipes' needs 😄🤓

@Wim Leers - I think we'll need a special autoloader for comparing configuration. It's going to be horrible! The better solution would be to completely remove Drupal's approach to autoloading. But this is a significant challenge.

#3205480: Drop PECL YAML library support in favor of only Symfony YAML is RTBC and looks like it'll land soon! 🤞

Wow, what a coincidence: minutes after posting that, #3205480: Drop PECL YAML library support in favor of only Symfony YAML actually landed! 😄

Merged in 3 months of upstream changes, most notably: resolved the conflict with #3205480: Drop PECL YAML library support in favor of only Symfony YAML (hopefully correctly).

@alexpott's work on this MR was already demonstrating very clearly how much this helps improve the config validation DX. So tagging and bumping priority.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Added some small changes:
- Cleanup unused data provider
- Don't require yaml extension for tests
- Cleanup mentions of Pecl in doc comments
- Symfony doesn't accept !php/const for the value of a backed enum (BackedEnumValue::Maybe->value)

Restoring priority

Thanks for fixing my conflict resolution mistakes, @gapple! 😄🙏

I think we need to address #53 before we allow this. I think we need a special autoloader that can only autoload enums from uninstalled modules that we activate in the ConfigImporter. If we don't do this then we're going to give people an easy way to break a site.

I missed that 😬 🫣

#46 provides a good description of the edge case. We should start by adding test coverage for the exact scenario described in #46, and that test will fail with the current MR.

Yeah, that seems pretty important since my understanding is that's the only place we'd be supporting this this right? Managing dependencies in the active store or exports sounds like an impossible task that would largely undo the consistency promises of config.

This blocks #3402178: [PP-1] Enum cases stored in config, which blocks #3422862: Add validation constraints to book.settings.

I tried to add a test in previous commit, which should fail, but it is not failing. 🤷‍♂️

@narendraR the test autoloader is pure evil. It needs to be a functional test and you need to do the config import via the UI on the system-under-test.

@narendraR Would be great if you could push this forward again 😇

Functional tests are failing in _install_get_version_info($version) for $version=11.0-dev in current MR and passing in 11.x 🤯. I think it needs unblocking from @alexpott 🙏

The tests are now fixed... sorry everyone I messed up implementing \Drupal\Core\Config\AutoloadingStorage::listAll() and didn't pass the prefix on to the underlying storage - which messed things up in a really confusing way.

We have added the missing test coverage that @Wim Leers identifier - i.e. test coverage of config import containing enums for class in modules that are not yet installed.

Lol, took forever to find the listAll() bug, and you fixed it before I could push :-)

This looks good to me and means our YAML can be more readable instead of copy-pasting constant values around.

https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co.... Found this too, while I was debugging the tests. Do we need to add the Autoloading storage here?

in the Storage Comparer:

  if ($source_storage instanceof FileStorage) {
      // FileStorage has its own static cache so that multiple reads of the
      // same raw configuration object are not costly.
      $this->sourceCacheStorage = new NullBackend('storage_comparer');
      $this->sourceStorage = $source_storage;
    }

Not the decision maker but just going to re-iterate, I think this is a bad idea. The hacks I see for uninstalled modules seem like just the tip of the iceberg. What about removed modules. removed libraries. uninstalled themed. profiles.... it seems like a loosing battle for little benefit.

@mikelutz we already get an autoloading storage here because it's getting the storage from config.storage.sync and we have test coverage in \Drupal\Tests\config\Functional\ConfigImportUITest::testEnumViaConfigImporter()

Right, that was my question. We now get an AutoloadingStorage here. Previously we got a FileStorage, and since FileStorage has it's own static cache, we were not setting a storage cache. Now we have an AutoloadingStorage, which is based on FileStorage and uses it behind the scenes, but we are setting the storage cache as well, since it's not a FileStorage. I was wondering if we needed to. As I was stepping through HEAD and the patch trying to find the difference that caused the bug in listAll it was one of the differences I noticed, as the importer was operating on a CachedStorage object in the patch versus a FileStorage object on HEAD. It wasn't the problem, but I'm wondering if we need the sourceCacheStorage with the Autoloading storage, or if we should carve out the same exception for AutoloadingStorage here that we have carved out for FileStorage.

It works either way, but I'm not certain which way is more performant, or if something new in the AutoloadingStorage makes the cached storage helpful again.

I don't want to block this, I just wanted to point it out and ask.

@mikelutz oh I see what you are on about. We are undoing the work in #3415821: StorageComparer should not wrap sourceStorage with a memory cache if it extends \Drupal\Core\Config\FileStorage - that's not good. Hmmm....

Just now getting into this issue, but I'm pretty spooked by @neclimdul's comment in #78. This has serious implications because, with this change as proposed, config schema can indirectly influence the dependency tree. That's a real danger zone.

If we're going to do this, then I think we need to have some kind of coherent fallback mechanism if a constant or enum cannot be loaded or parsed. It must fail gracefully.

Either that, or we should go in the opposite direction and it should be extremely strict, with clear rules about when a constant in YAML is valid or not, and what conditions need to be fulfilled for that.

It must fail gracefully.

I beg to differ. It must fail loudly and obviously. If you have removed code that your configuration expects this is a big error that Drupal soldiers on from time and time again causing more harm.

Then it has to be extremely strict, and exceptionally clear about what the rules are.

So, like @neclimdul, stuff like this is giving me pause:

    // Ensure enums and constants from uninstalled modules can be autoloaded.
    $storage = new AutoloadingStorage($storage, [$name => \Drupal::root() . '/' . $this->extensionPathResolver->getPath($type, $name)]);

What is the justification for allowing this kind of thing?

Because we need to be able to load configuration during config import before a module has been installed. NOTE: the AutolaodingStorage only applies to sync storage and not to active storage - so in my mind the risk is actually minimal and correct because it is exactly the time you expect code to change. FWIW there is no difference here if you try to import a config entity for a module and the entity class is not present. Once you run the import things are not going to work as you expect or want. It would be great if #2628144: Ensure that ConfigImport is taking place against the same code base and #2762235: Ensure that ConfigImport is taking place without outstanding updates had landed back when they were green years ago but people got far too used to being able to import incorrect versions config and began to see it as a feature rather than the bug that it is.

Not the decision maker but just going to re-iterate, I think this is a bad idea. The hacks I see for uninstalled modules seem like just the tip of the iceberg. What about removed modules. removed libraries. uninstalled themed. profiles.... it seems like a loosing battle for little benefit.

I'm not fully seeing the issue here. What do you mean about removed modules, removed libraries, etc? If you uninstall/remove code providing a constant/enum used in your active configurations, you will have problems, as you should. The 'hack' here only adds a module to the classloader as it's config is read in preparation to install it. This seems appropriate to me, as the config you are installing should be able to use enums defined in the module you are installing. This isn't about loading enums and consts from uninstalled modules, this is to load enums and consts from a module that we are in the process of installing. I really think the inline comments are scarier than what the code is doing, and I made a couple suggestions on the comments.

I also added a test here that I think shows another issue, config referencing constants that are in the modules .module file. I think we will have to include the .module file before parsing the config as well.

Also, I tried to find a more elegant way to prevent double caching of the autoloading storage, but I really couldn't, so I just added a second instanceof check in the Comparer. I don't like it.

Merged wit 11.x HEAD, resolved conflicts and addressed PHPCS and PHPStan errors. Would love to see this in.

This is now failing with

Drupal\Core\Config\UnsupportedDataTypeConfigException: Invalid data type in config config_enum_test.settings, found in file core/modules/config/tests/config_enum_test/config/install/config_enum_test.settings.yml: The constant "CONFIG_ENUM_TEST_VALUE" is not defined at line 2 (near "bar: !php/const CONFIG_ENUM_TEST_VALUE"). in Drupal\Core\Config\FileStorage->read() (line 118 of core/lib/Drupal/Core/Config/FileStorage.php).

I suppose because CONFIG_ENUM_TEST_VALUE is a global const defined in config_enum_test.module, but the module file is not preloaded.

I suppose because CONFIG_ENUM_TEST_VALUE is a global const defined in config_enum_test.module, but the module file is not preloaded.

That is the test I added in #87 to show the issue regarding loading the module file. It hasn't been addressed yet in the MR

Whops… ok.

Is it really worth supporting global const defined in .module files as we speak? Could we not assume OOP consts only?

I'm not opposed to documenting that limitation and moving on without it. Alternatively, we could just load the module files prior to installation, same as we are now doing with the classloader. I didn't code up any solutions because I wanted some comments on whether we should worry about it at all. I mainly wanted to surface the issue, as we did the work on the classloader to ensure OOP constants and enums in a module's src folder were loaded and accessible during config install.

Also, to clarify for anyone reading this in the future, constants in .module files of active modules do work. The issue here is specifically happening when installing a module whose install config references a constant defined in the .module file. It's only during module install where a module's config files are loaded without the .module file being loaded.

Rerolled patch to use with 10.4.x core

This issue might be suitable to also update \Drupal\Component\DependencyInjection\Container::getParameter to allow enum returns.

Method signatures

\Symfony\Component\DependencyInjection\ContainerInterface::getParameter

public function getParameter(string $name): array|bool|string|int|float|\UnitEnum|null;

\Drupal\Component\DependencyInjection\Container::getParameter

public function getParameter($name): array|bool|string|int|float|NULL {

--

And somewhat related, looking to address use of enums [and object instances] in parameters throwing fatal errors since autoloader isnt fully populated and unserialisation is attempted, at @ #3522410: Using instances of classes defined by modules in service container parameters causes fatal errors

#93 and #94 IMHO the effort to try supporting global constants in unloaded .module files is not worth doing at this point. So dropped it in the last commit.

Retitled and updated IS.

Based on recent comments it looks like this one is pretty close to being ready. I made a couple of suggestions to change the comments that are causing some of the recent concern.

Applied suggestions, thanks.

I double-checked the feedback on the MR since this was last RTBC in #81 and it's all been addressed.

It's a lovely present for 2026 that this issue is RTBC - but unfortunately I think we have to add some test coverage and resolve the @todo we're adding in the code.

Added some review comments, but also converted the setValue() validation to a constraint including tests, and converted the Enum data type to use PrimitiveBase.

Disclaimer: some of the changes in the last commit were created by Google Gemini, then reviewed and tweaked by hand.

Appears to have some open threads? @alexpott thoughts on the test coverage added?

Merged with main.

The functionality of the test looks good. I had one comment on the implementation.

I'm mostly setting the status to Needs Work because of the unresolved comment from @longwave on core.services.yml.

The test that is failing should now be green again (it is on my local machine), I've also fixed the remark from fabian in this issue.
We still need to look at the remark from @longwave.

I don't really agree with the request to name it back to the original names, to me this is a lot clearer. I think this is good to review.

Added the deprecation for the class as well as the test for installing a module that has a reference to an Enum that is installed in a different module.
This test was generated by AI after @bircher and I wrote the prompt together at dev days yesterday.

The last comment, about the RecipeConfigStorage, I'm not sure about.

Nice!
I think if we install the two modules in a test also via a recipe we will know if the recipe system can deal with it too.

Ok, now I didn't find anything more to complain about I think I can give this a RTBC status again.

reviewed the last few commits again, and still RTBC

Agreeing with @bircher, I looked at the last couple of commits as well, those look great.

Added some very small nits but this looks great so far!

Fixed all the very small nits. As these were very small moving back to RTBC

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Couple of questions on the MR - mostly about code comments, but they're code comments about the trickiest part of this.

Thanks for the review @catch. Given both were about comments setting back to rtbc... responded to both comments on the MR and added commentary for one of them.

I read both change records. The one about how to write constants and enum in yaml file is fine The one about FileStorageFactory I found hard to read. I reworded it but there is one sentence that is difficult. It is "Using the sync storage service ensures any configuration for modules or themes that are not installed will be able to use constants and enums from these modules". I am just too jet lagged to improve that one. Maybe someone else can do that. And a review of it to make sure that it is still correct would be good.

I have updated credit.