🇳🇱Netherlands @bbrala

Adjusted it to actually do what was asked for.

Did revert the deprecation message for the path property. Would like to know if we need to make the change here, or if we should defer that. Seems like that has been here for quite a while. I do think we might be able to ignore it here, since it doesnt seem to be used.

Waiting for build, since i had some cs issues ;x

So, the path config key was deprecated in Drupal 8.8 it seems ( https://www.drupal.org/node/3039255 → ). That kinda feels out of scope of this change, the fact is that it will need to go through deprecation? I don't even really see any usage in core, but that could be my search skills failing me.

But i don't feel we should do that here. Hopefully you agree.

Fixed the phpstan issue.

Also;

1st comment is Wim telling what this brings us, which is not an issue to resolve.

@quietone did leave feedback, but later wanted to commit so feel like she didnt feal to strong about it. Keeping is open to get extra attention from the committer here.

ModuleHandler already uses exists to check for installed. So the naming is kinda consistend.

If we need to seperate why now;

ExtentionAvailable, which would be consistent with things like; 'update available' or perhaps 'moduel available to install'. Doesn't feel that weird to me.

Updated the class namespace as suggested, did do a rebase to clean up.

bbrala → made their first commit to this issue’s fork.

Did a codestyle fix and a rebase, since it was behind so much. That will create issues with lint jobs.

bbrala → made their first commit to this issue’s fork.

Think your last comment does seem reasonable, guessing we could work with that.

Added 📌 Add validation constraints to views.settings Postponed since this blocks that through a jump issue that was closed as 'duplicate' of this.

Seems blocked on 🐛 views.settings config object should not be used to cache list of available plugins Active since that one is the one still open, the blocked listed earlier was closed as duplicate of 🐛 views.settings config object should not be used to cache list of available plugins Active

Rebased and updated one deprecation message to 11.2.0 since we moved past 11.1.0

bbrala → made their first commit to this issue’s fork.

Rebased thsi issue.

@alexpott

> I think this issue points to us needing a new constraint which is something like NotBlankAfterInstall - so a very can be blank during site install but once the site is up and running it can not be. This is true for uuid, name and email.

You say this, which is kinda the same problem as 📌 Add validation constraints to contact.form.* Needs work comment #32. There it is a recipe which wants to use mail to replace a value, which is doesn't get since mail is not yet in config when installing from a recipe with the drupal command.

So there is also some sort of 'during install' issue, where during an install is appearantly possible to have invalid config.

Perhaps we even need to fix it ealier? Although i don't really feal like ignoring validation for config while installing is a good idea. Since it is pretty valuable, what if you are to install some sort of Drupal implementation (like a decoupled Drupal) and it comes with invalid config, would we want to ignore that?

Somewhat related: 📌 Add validation constraints to system.site Needs work

bbrala → made their first commit to this issue’s fork.

Not too sure man. Although anything is possible right now looking at contrib (core doesnt really do much here) i see stuff like:

'indexes' => array(
      'target_id' => array(
        0 => 'target_id',
      ),
    ),

      $fields[$field_name]->setIndexes(['value' => ['value']]);

$field_storage->setIndexes([
    'value' => ['value']
  ]);

(url : https://git.drupalcode.org/search?group_id=2&scope=blobs&search=-path%3A...)

And not much more.

I feel like i'm boucing between, it should be possible, and its not worth it every time i read about this. So perhaps i should defer to those who are into this a bit more.

Ok, added a cr

tried a few different ways to get this to work. I couldn't, its way to diverse imo.

Added the comment.

Hmm, was thinking while writing the comment. It could be validated to be an array. It is optional, but seems to always be an array. The array shape is extrmely flexibly. But an array nevertheless.

Title updated.

I looked at other issues regarding validation, and when there is no change in the interface (like '' becoming NULL) it seems it is not needed to have a CR as far as i can see.

Added CR

Yeah cr, is, and title updtae. Since this basically is the implementation of this constraint.

Still failting on the standard install. Which has pretty much the. same problem, but unfortunately, does not configure system.site.mail at all.
In the test:

    // Standard expects to set the contact form's recipient email to the
    // system's email address, but our feedback_contact_form recipe hard-codes
    // it to another value.
    // @todo This can be removed after https://drupal.org/i/3303126, which
    //   should make it possible for a recipe to reuse an already-set config
    //   value.
    ContactForm::load('feedback')?->setRecipients(['simpletest@example.com'])
      ->save();

Which i alerady opened an issue for earlier. 📌 #3303126 was fixed, but a todo was not removed. Active . So guessing this might need to use another angle. Not sure what.

Updated the comments. :)

Some discussion on the issue with recipes here: https://drupal.slack.com/archives/C2THUBAVA/p1743182816947909

In order to load default values for recipes when quickstart is used we need to make sure default values that use config are actually found. We do this using a lookup array for the config path to $install_state.

Unfortunately the default email address for an install needed changing also since drupal@localhost is not a valid email address. I changes this to @drupal.local

Some more digging into the feedback form recipe.

It seems the actions are applied, BUT InputConfigurator::collectAll is never called, so while installing it seems like it doesn't really collect anything from config, so the placeholder ${recipient} is never replaced, therefor we get install errors.

FOund when debuging the failing test, stepping through RecipeRunner and seeing the $config_action_manager->applyAction($action_id, $config_name, static::replaceInputValues($data, $replace)); didnt really have any input values.

So basically this part of therecipe:

input:
  recipient:
    data_type: email
    description: 'The email address that should receive submissions from the feedback form.'
    constraints:
      NotBlank: []
    prompt:
      method: ask
      arguments:
        question: 'What email address should receive website feedback?'
    form:
      '#type': email
      '#title': 'Feedback form email address'
    default:
      source: config
      config: ['system.site', 'mail']

Is never looking up the default value, therefor we enter a invalid value when the config is installed.

Opened 📌 #3303126 was fixed, but a todo was not removed. Active since that should fix one of the tests. The references issue in the failing test tells us it was fixed and some code could be remove.d.

As per todo, removed the code.

bbrala → created an issue.

From FieldItemInterface:

   *   - indexes: (optional) An array of Schema API index definitions. Only
   *     columns that appear in the 'columns' array are allowed. Those indexes
   *     will be used as default indexes. Field definitions can specify
   *     additional indexes or, at their own risk, modify the default indexes
   *     specified by the field-type module. Some storage engines might not
   *     support indexes.

I think this is overkill really to validate. I would vote to skip trying to validate this.

There are posiiblities to at least check if the schema is part of the columns. But don't really seem much value in that.

Thinking about this i think you are right. Doesn't seem to make sense. Closing.

Adding related issues because that is helpfull, but also to test some things.

I kinda agree that a noop might not really be a problem.

#224480: Transactions from User Points Basic showing "insert" as operation → was merged

Open issue to test some things.

I did a bugfix for the validator in the parent issue: https://git.drupalcode.org/project/drupal/-/merge_requests/3047/diffs?co...

That change is needed here also.

This was a clean rebase. Not sure why its confused.

Although a small question regarding the sleep, although this seems sensible. Don't see any issues here. Does remind me of using metrics.txt for this in gitlab.

quietone → credited bbrala → .

I've split two issues for the missing tests.

1. ✨ Implement MatchesOtherConfigValue tests Active
2. ✨ Implement NoEntitiesExistYetWithHigherCardinality tests Active

Added the changes from the parent, lets get some working tests.

Cherry picked the change, we need to make tests.

bbrala → created an issue.

bbrala → created an issue.

Fun fact is that indexes have a default, like value => [value], but that the shape can be defined in the childs, therefor this is really hard to reallt validate.

Although there is some missing test:

1. The constraint MatchesOtherConfigValue needs tests
2. The constraint NoEntitiesExistYetWithHigherCardinality needs tests

Our tests are green :x

Rebased, lets see if this is still green.

bbrala → made their first commit to this issue’s fork.

bbrala → created an issue.

bbrala → created an issue.

bbrala → created an issue.

esmoves → credited bbrala → .

esmoves → credited bbrala → .

I would advise we'd also add an optinal argument limit. So you can fetch x of the overview and perhaps make sure people dont start using this and end up querieing thousands of nodes.

This seems good, went through logs and didnt really notice anything wrong. We (castch, mstrelan) have discussed how one would run the child pipeline with failing unit tests, for now manually adding allow failure to the unit tests will be fine. Setting to RTBC.

Conflict was on cspell word list. Rebased, keeping rtbc

Seems nicely rebased, back to RTBC

There is agreement on closing this, as per docing standars meeting feb 25th

I'll try and dive into this sometime soon.

One thing that I did notice, there were some conceirns in #77, are those valid or handled?

All green.

Did some digging. Ended up removing the exception and checking for null and '' in IsCommentable and EntityTypeExists. Seems to make more sense. Also adjusted the test so it expects extra validation errors when looking for null.

Well, 2 test failures:

1. core/recipes/feedback_contact_form/recipe.yml -> The receipient input is not set when installing in core/tests/Drupal/Tests/Core/Recipe/RecipeQuickStartTest.php, so it fails since ${resipient} is not a vaild email adress.
2. core/tests/Drupal/FunctionalTests/Core/Recipe/StandardRecipeInstallTest.phphas the same issue, no recipient set.

Merged 11.x into this, lets see what happens.

bbrala → made their first commit to this issue’s fork.

Ok, so the issue remaining is the fact that in ConfigEntityValidationTestBase::testRequiredPropertyValuesMissing it tries to test what happens when a property is set to null. When EntityTypeExistsConstraintValidator is validated it also executes the code:

    if (!is_string($value)) {
      throw new UnexpectedTypeException($value, 'string');
    }

Which is then thrown. When looking at other implementations where this eror is thrown i see ContentLanguageSettings with EntityBundleExists. This sems to be an immutable property.

I'm now guessing there is 2 possible solutions:

1. The target_entity_type_id property of the comment type should be immutable.
2. It is not immutable, but instead of throwing an UnexpectedTypeException is might just need to add a validation error that the property shhould be string.

What the right solution is, i dont know. Hopefully someone does :x

I don't get why the id cannot be string and this test fails. Trying to figure why that would not be allowed. The schema does tell us string is fine, is that wrong then?

Drupal\Tests\comment\Kernel\CommentStringIdEntitiesTest::testCommentFieldNonStringId
Failed asserting that exception of type "Drupal\Core\Config\Schema\SchemaIncompleteException" matches expected exception "UnexpectedValueException". Message was: "Schema errors for comment.type.foo with the following errors: 0 [target_entity_type_id] The 'entity_test_string_id' entity is not commentable" at
core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php:98
vendor/symfony/event-dispatcher/EventDispatcher.php:206
vendor/symfony/event-dispatcher/EventDispatcher.php:56
core/lib/Drupal/Core/Config/Config.php:230
core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:260
core/lib/Drupal/Core/Entity/EntityStorageBase.php:487
core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:239
core/lib/Drupal/Core/Entity/EntityBase.php:370
core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:618
core/modules/comment/tests/src/Kernel/CommentStringIdEntitiesTest.php:53

Still some test errors left in biuld/jsonapi and randomjs kinda test.

Seems a test is still fialing since it expects a string, not null.

Commentable makes so much sense i dont want to change it, just added to the drupal words. THink this can go back to NR

Next up, this needs a little word making sure the fixture/config* is updated to reflect some of the config changes.

bbrala → made their first commit to this issue’s fork.

bbrala → made their first commit to this issue’s fork.

Well at least tests are close to OK again so this can be worked on again.

As per Alex, we need a comment there, i understand it feels fragile, unfortunately a result of how things are run.

Todo: re-addthe creation of the entity types removed in one of last 2 commits

Seems good enough, moving to main MR, shoudln't need 'Needs review' :)

Scary rebase, so made a new branch first to see how it handles itself.

bbrala → made their first commit to this issue’s fork.

Seems fixtures might need work. Not sure how those work.

Removing wim from assignment, that will probably not make this go faster.

My conclusion in #27 is that null is a good idea. I rebased and if tests don't fail I think this can move forward as is. Since my additions were minimal, setting RTBC.

Going through the config issues a bit to see where things are hanging.

I read through all this, and have not seen any discussion around the NULL change. I understand the text in the CR ("The reason is because an empty string makes no sense for this field. "" is never a useful description of a form mode."). But I dont really see why this is as much better as it is a change.

If i try to argue why it should be null: Now i want no desription is equal to en empty description. This unfortunately meant he field is not really optional, when you wouldn't post an description you technically tend a NULL, which cannot be stored right now. But if we allow null, we would be able to save without the field, since null is fine.

This could have reasons in jsonapi, maybe we can then post without the field and make it null, instead of requiring an empty string?

This is as far as i get.

Added a test for the validation. Removed an extra comment and added possible follow up ✨ [Follow-up] action.configuration.user_remove_role_action could also use a UserHasRole constaint Active

bbrala → created an issue.