- Merge request !2726Issue #2951046: Allow parsing PHP constants in YAML files â (Open) created by Eduardo Morales Alberti
- ðŽð§United Kingdom alexpott ðŠðšð
This is lovely. It'd be great to add supported to enums at the same time.
- ðŽð§United Kingdom alexpott ðŠðšð
We can extend \Drupal\Tests\Component\Serialization\YamlTest::testYamlFiles() to add explicit test coverage.
- Status changed to Needs review
7 months ago 12:17pm 17 November 2023 - ðŽð§United Kingdom alexpott ðŠðšð
This will allow us to use enums as container arguments which would have been lovely for https://www.drupal.org/project/drupal/issues/3402168 ð Follow-up for #3361534: config validation errors can still occur for contrib modules, disrupting contrib Active
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.
- Status changed to Needs work
7 months ago 1:20pm 17 November 2023 - ð§ðŠBelgium Wim Leers Ghent ð§ðŠðŠðš
This is looking great already! ðĪĐ
- Status changed to Needs review
7 months ago 2:29pm 17 November 2023 - ðŽð§United Kingdom alexpott ðŠðšð
Our test coverage of this change is not create until #3402448: Add PECL Yaml to PHP 8.1 and PHP 8.2 â is fixed.
- ðŦð·France andypost
Created https://git.drupalcode.org/issue/drupal-3386474/-/pipelines/51765 to test new dev images with YAML extension enabled
- ðŦð·France andypost
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).
- ðŽð§United Kingdom alexpott ðŠðšð
opened ð YamlFileLoaderTest fails if you have PECL yaml installed Needs review to fix the HEAD fail with PECL yaml.
- First commit to issue fork.
- ðšðļUnited States smustgrave
Just rebased since the issue in #38 was merged.
- Status changed to Needs work
7 months ago 3:01pm 20 November 2023 - ðšðļUnited States smustgrave
See it was tagged for a CR, so only moving to NW for that.
- ðšðļUnited States neclimdul Houston, TX
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.
- ðŽð§United Kingdom alexpott ðŠðšð
@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.
- ðšðļUnited States neclimdul Houston, TX
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.
- ðŽð§United Kingdom alexpott ðŠðšð
@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 beBackedEnumValue::Maybe->value
and notBackedEnumValue::Maybe
for a comparison of values. Which would fail.... - Status changed to Needs review
7 months ago 9:45pm 23 November 2023 - ðŽð§United Kingdom alexpott ðŠðšð
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.
- ðŽð§United Kingdom alexpott ðŠðšð
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.
- Status changed to Needs work
7 months ago 8:52am 24 November 2023 - ð§ðŠBelgium Wim Leers Ghent ð§ðŠðŠðš
This looks very close, but I think we need a bit more test coverage to ship this with full confidence.
- ðŽð§United Kingdom alexpott ðŠðšð
@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. - ð§ðŠBelgium Wim Leers Ghent ð§ðŠðŠðš
- 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 ð
- [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
. - 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:
- Symfony will only encode enums using ONLY
!php/const
- 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 ð - Symfony will only encode enums using ONLY
- ð§ðŠBelgium Wim Leers Ghent ð§ðŠðŠðš
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 ððĪ - ðŽð§United Kingdom alexpott ðŠðšð
@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.
- ð§ðŠBelgium Wim Leers Ghent ð§ðŠðŠðš
ð PECL YAML library supports YAML 1.1, but Symfony supports YAML 1.2 Active is RTBC and looks like it'll land soon! ðĪ
- ð§ðŠBelgium Wim Leers Ghent ð§ðŠðŠðš
Wow, what a coincidence: minutes after posting that, ð PECL YAML library supports YAML 1.1, but Symfony supports YAML 1.2 Active actually landed! ð
- Status changed to Needs review
4 months ago 5:00pm 7 March 2024 - ð§ðŠBelgium Wim Leers Ghent ð§ðŠðŠðš
Merged in 3 months of upstream changes, most notably: resolved the conflict with ð PECL YAML library supports YAML 1.1, but Symfony supports YAML 1.2 Active (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.
- Status changed to Needs work
4 months ago 5:36pm 7 March 2024 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.
- Assigned to Wim Leers
- First commit to issue fork.
- Issue was unassigned.
- Status changed to Needs review
4 months ago 7:12am 9 March 2024 - ðĻðĶCanada gapple
Added some small changes:
- Cleanup unused data provider
- Don't require yaml extension for tests
- Symfony doesn't accept!php/const
for the value of a backed enum (BackedEnumValue::Maybe->value
) - Status changed to RTBC
4 months ago 7:50am 11 March 2024 - ð§ðŠBelgium Wim Leers Ghent ð§ðŠðŠðš
Thanks for fixing my conflict resolution mistakes, @gapple! ðð
- Status changed to Needs work
4 months ago 8:54am 11 March 2024 - ðŽð§United Kingdom alexpott ðŠðšð
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.
- ð§ðŠBelgium Wim Leers Ghent ð§ðŠðŠðš
- ðšðļUnited States neclimdul Houston, TX
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.
- ð§ðŠBelgium Wim Leers Ghent ð§ðŠðŠðš
This blocks âĻ [PP-1] Enum cases stored in config Postponed , which blocks ð Add validation constraints to book.settings Needs review .
- First commit to issue fork.
- ðŪðģIndia narendraR Jaipur, India
I tried to add a test in previous commit, which should fail, but it is not failing. ðĪ·ââïļ
- ðŽð§United Kingdom alexpott ðŠðšð
@narendraR the test autoloader is pure evil. It needs to be a functional test.
- ð§ðŠBelgium Wim Leers Ghent ð§ðŠðŠðš
@narendraR Would be great if you could push this forward again ð
- ðŪðģIndia narendraR Jaipur, India
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 ð