🇳🇱Netherlands @bbrala

Hi! Thanks for working on this. I think we should try and find the reference from the parent as seen here: https://git.drupalcode.org/project/drupal/-/merge_requests/13004/diffs#c...

That makes it actually dynamic, right now you just do the same, but then split in two which i think doesnt add much value.

Rebased

Directory name Contraint vs Constraint. I keep making that typo... ;x

Needs some small fixes in test names and such ot seems.

Wow just wow, i think i mixed code because there was indeed no test.

Created a test that validates, and also updated the logic.

delta == count -1, which was not added, so it worked, but only after having 2 extra for the cardinality!

Also rebased.

I'm all for sinplifying the workflow, co-authored-by should be fine. I do think we are overly complicating things by giving co-authored-by so much extra weight. I think for an oblivious person it really means the same as the old format. "Issue xxxx by bbrala, catch" reads the same. "This issue has come together by these people" as co authored suggests also.

Let's not pick a standard, and then move away from that standard too much just because we can explain the words differently.

Also, wouldn't an possible endgoal be that we generate the commitmessage from gitlab as much as possible? There are variables there to do a lot of heavy lifting.

https://docs.gitlab.com/user/project/merge_requests/commit_templates/

That was also part of my frame of reference when discussing the commit message change. Then we don't need to manually do much, but that does mean, dont invent our own. But I guess that part was not iterated much on (or perhaps even mentioned explicitly enoug).

📌 Validate configuration schema before importing configuration Needs work

Might just be it is just not yet done on config import, but only at save.

bbrala → created an issue.

bbrala → created an issue.

Tried a few things to get FieldConfigValidationTest to work, didnt get it to do what i like. Cannot mutate id field, and not getting later errors there. Even tried to remove the Immutable property, but didnt get it there.

I added a test for the message in the testStringPartsConstraint so i covered it from that side i guess?

I was hoping for API revisions, but seems those are not available. Would a revision history from the database be possible perhaps @drumm think we only need: title, body, revision date, user (maybenot)? Converting html to MD is easy, and if we have a csv with the revision history it should be quite easy.

Loading and copy pasting each revision is a little more effort :p

I've made 🌱 [META] Update schema's with multiple contraints to SequentiallyConstraint Active to start working on those after this issue.

bbrala → created an issue.

Thanks for the review :) I'll have a go to update test.

Also see if i can create some followups for

• EntityViewMode
• EntityFormMode
• EntityViewDisplay
• EntityFormDisplay

Valid point. As per parent Wim said he had a quite a few places where this was usefull, but he didnt say where. When i look at core i see a few places where this might be quite usefull. Few examples:

# file: block.schema.yml
    theme:
      type: string
      label: 'Theme'
      constraints:
        NotBlank: []
        ExtensionName: []
        ExtensionExists: theme

Here it could first say "Should not be blank", then if that is true "Invalid extension name", when "Extension does not exist".

This is way better than doing them all.

Same here:

# file: block.schema.yml
    region:
      type: string
      label: 'Region'
      constraints:
        NotBlank: []
        Callback: ['\Drupal\block\Entity\Block', validateRegion]

or

# Plugin \Drupal\ckeditor5\Plugin\CKEditor5Plugin\Language
ckeditor5.plugin.ckeditor5_language:
  type: mapping
  label: 'Language'
  constraints:
    FullyValidatable: ~
  mapping:
    language_list:
      type: string
      label: 'Language list ID'
      constraints:
        # Configuring this does not make sense without the corresponding button.
        CKEditor5ToolbarItemDependencyConstraint:
          toolbarItem: textPartLanguage
        # Only the following values are accepted.
        Choice:
          # United Nations "official languages".
          - un
          # Drupal's predefined language list.
          - all
          # Languages configured at /admin/config/regional/language for the site.
          - site_configured

I think a lot of the places where more than one constraint is defined we should use this. Should we pick one or perhaps just do follow ups? What do you think.

You are right. If we want to check for fieldable, we should actually check for that interface. Adjusted. Setting to NR (although tests are running still).

Another option would be to migrate the history to the pages, just commit from start, maybe even add real commit date and work from there. Although what catch suggests is a lot less work, so wouldn't really mind that. It might mean some issues when we migrate to a new site though, so my preference would be to do the work of creating the history in git.

I think, asssuming we use markdown, this could just mean move through history, copy html, convert to markdown, add proper commit message. Next.

Resolved all the comments and made some small changes (all naming), hopefully it will be green. Skipping back to rtbc since changes where nothing special.

TEst failure is unrealted, restarted tests, but will be bold and set back to rtbc.

Rebaed, but still needs work.

Gonna try again.

Failures seem to have been unrealated. Setting back to RTBC

Seems like a good suggestion to add that.

Added the extra removal, and rebased while i was at is.

Thanks quietone for the review. Since it was all style/wording ill set it back to RTBC

I dont fully remember how it works for config import right now. The test would set the value and validate in config save, which should trigger. What could be happening also is when your config has something higher than max int it get clipped to max? But that is speculating.

Only unrealated failure on the database left! Yay!

Only 2 failing tests it seems, because of missing adjustments in the tests using config_test. I added a new key, and that resulted in some changes in the TypedConfigTest

Asjusted the expected interface for the entity type. Entitytypemanager returns the Entity class, not the type class. So changed the interface we check for.

Also was missing a required option in the config_test.

Sorry for the late reponse. I think that would be ok to tranparantly do this. Don't see how that would create issues. Clients will be OK, only see possible issues in tests which would do a full compary of the output. Doesn't seem like a problem.

I discussed the fact we are adding 0 to existing also with casey. We do think this will not result in issues since its in the meta. Also looking at how the count is done, that also seems fine.

What I'm kinda wondering, we should probably also have at least some sort of test for the meta data. Seems we can so that easily here? Perhaps even patching with artity also (although maybe not for every entity type) to see if that works?

file: core/modules/jsonapi/tests/src/Functional/JsonApiFunctionalTest.php

    // 6. Single relationship item.
    $single_output = Json::decode($this->drupalGet('/jsonapi/node/article/' . $uuid . '/relationships/node_type'));
    $this->assertSession()->statusCodeEquals(200);
    $this->assertArrayHasKey('type', $single_output['data']);
    $this->assertArrayNotHasKey('attributes', $single_output['data']);
    $this->assertArrayHasKey('related', $single_output['links']);
    // 7. Single relationship image.
    $single_output = Json::decode($this->drupalGet('/jsonapi/node/article/' . $uuid . '/relationships/field_image'));
    $this->assertSession()->statusCodeEquals(200);
    $this->assertArrayHasKey('type', $single_output['data']);
    $this->assertArrayNotHasKey('attributes', $single_output['data']);
    $this->assertArrayHasKey('related', $single_output['links']);
    // 8. Multiple relationship item.
    $single_output = Json::decode($this->drupalGet('/jsonapi/node/article/' . $uuid . '/relationships/field_tags'));
    $this->assertSession()->statusCodeEquals(200);
    $this->assertArrayHasKey('type', $single_output['data'][0]);
    $this->assertArrayNotHasKey('attributes', $single_output['data'][0]);
    $this->assertArrayHasKey('related', $single_output['links']);
    // 8b. Single related item, empty.

Added casey because we sat down together to disect the possible implications.

Awesome!

Few things, codestyle is failing right now, would be nice to get test runs :)

Also; perhaps what we do with the message in Drupal\Core\Validation\Plugin\Validation\Constraint\Range is relevant.

In regard to testing, you can use config_test module perhaps? I used that in ✨ Make the `field_config_base` structure fully validatable Active to test if config triggers an error and seems like an easy way to add a test.

<?php

declare(strict_types=1);

namespace Drupal\KernelTests\Core\Validation;

use Drupal\KernelTests\KernelTestBase;
use PHPUnit\Framework\Attributes\Group;

/**
 * Tests the UriHost validator.
 */
#[Group('Validation')]
class StringPartsConstraintValidatorTest extends KernelTestBase {

  /**
   * {@inheritdoc}
   */
  protected static $modules = ['config_test'];

  /**
   * {@inheritdoc}
   */
  protected function setUp(): void {
    parent::setUp();

    $this->installConfig('config_test');
  }

  /**
   * @see \Drupal\Core\Validation\Plugin\Validation\Constraint\UriHostConstraint
   */
  public function testUriHost(): void {
    $typed_config_manager = \Drupal::service('config.typed');
    /** @var \Drupal\Core\Config\Schema\TypedConfigInterface $typed_config */
    $typed_config = $typed_config_manager->get('config_test.validation');

    // Test valid names.
    $typed_config->get('string_parts')->setValue('localhost.llama');
    $this->assertCount(0, $typed_config->validate());

    // Test invalid names.
    $typed_config->get('string_parts')->setValue('drupal.kitten');
    $this->assertCount(1, $typed_config->validate());

  }

}

Added latest code from XB, unfortunately for now we are failing on PluginExists suddenly. Not sure why yet.

Added the missing contraint and a basic first test.

This has been fixed ages ago, sorry :)

https://git.drupalcode.org/project/project_analysis/-/merge_requests/34

This is the merge request with the work.

First i made the changes in d11 so i knew tests are green in the new code. Then migrated the code to de d12 branch and worked on more edgecases.

bbrala → created an issue.

So guess this is kinda a child of that :x

Unfortunately a little optimistic:

📌 Convert field_storage_config and field_config's form validation logic to validation constraints Needs work found it here. Not yet part of core.

Yeah i agree, would be enough.

Would be awesome to make the rows clickable also i think :)

resolved all feedback. There is a test failure. But that seems unrelated.

Installer Isolation Level Existing Settings (Drupal\Tests\mysql\Functional\InstallerIsolationLevelExistingSettings)
     ✘ Installer
       ┐
       ├ Failed asserting that two strings are equal.
       ┊ ---·Expected
       ┊ +++·Actual
       ┊ @@ @@
       ┊ -'REPEATABLE-READ'
       ┊ +'READ-COMMITTED'
       │
       │ /builds/issue/drupal-3441503/core/modules/mysql/tests/src/Functional/InstallerIsolationLevelExistingSettingsTest.php:58
       ┴

bbrala → created an issue.

Is this a dupe, or more correctly, a predecessor? :)

🌱 [policy] Decide on format of commit message Active
📌 Change format for git commit message Active

Not sure where you got StringParts from here?

bbrala → created an issue.

Context is great to have available there yeah, i do see a test failure though, which does seem to point to an issue?

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

Doing some housekeeping.

Awesome! Thanks so much for this fix.

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

Thanks all! This is looking great.

Some good improvements, thanks alex.

Closed as duplicate, but the link is this issue? Seems like a small error there.

<3

esmoves → credited bbrala → .

smustgrave → credited bbrala → .

smustgrave → credited bbrala → .

smustgrave → credited bbrala → .

Project analysis is close to working, having some commit to result branch issues which i need to flesh out. This unfortunately kinda means im not 100 sure how the data is right now. It is available as an artifact of the new master-d12 job.

https://github.com/palantirnet/drupal-rector/releases/tag/0.21.0

Has been merged. Awaiting release.

Wow, preparing a session and founsd this... That did not end well, sorry!

Although hard, voting for;

- Joris Vercammen (borisson_)
- Marine Gandy (Mupsi)

No weird changes, actual green tests in main MR and also the test mr with updates package. Nice :)

Updated lock file hash.

https://drupal.slack.com/archives/C079NQPQUEN/p1747131446995239 slack thread

Added the requested docs.

bbrala → created an issue.

bbrala → created an issue.

Mis understood, RTBC was referring to the change record.

Updated CR

I normally search like this: https://git.drupalcode.org/search?group_id=2&scope=blobs&search=-path%3A...

Since when are we opening issues on contrib projects for deprecations/removal? I've not seen that before :)

Oh, will rebase later today.

Bot is broken

Rebased

Edited because LSEP character in title is no fun to handle when using the API and import the data into a neo4j database.

Still open comments on the MR, those need adressing. :)

All green and happy after the rebase.

Made the few small changes you asked for, setting RTBC, all seem straightforwards enough and tests are still green. Updating Cr right now.

Never mind me, all good, that is the branch which uses 6.x to test. Adding ignore so tests run, but nothing to worry about. Keeping RTBC

Seems we have an issue left here after rebase:

------ ------------------------------------------------------------------------- 
  Line   core/tests/Drupal/Tests/Core/Theme/Component/ComponentValidatorTest.php  
 ------ ------------------------------------------------------------------------- 
  351    Call to an undefined static method                                       
         JsonSchema\ConstraintError::FORMAT_URL().                                
         🪪  staticMethod.notFound                                                
  355    Method JsonSchema\Constraints\BaseConstraint::addError() invoked with    
         4 parameters, 1-3 required.                                              
         🪪  arguments.count                                                      
 ------ ------------------------------------------------------------------------- 

This is the changelist:
https://git.drupalcode.org/project/coder/-/compare/8.3.28..8.3.29?from_p...

There are changes to that sniff it seems.

I would suspect: https://git.drupalcode.org/project/coder/-/commit/78034f0a41956c4f8d3808...

THis is the changelist:
https://git.drupalcode.org/project/coder/-/compare/8.3.28..8.3.29?from_p...

There are changes to that sniff it seems.

I would suspect: https://git.drupalcode.org/project/coder/-/commit/78034f0a41956c4f8d3808...

Ty and merged. Nobnew release though, do younneed one?

Confused, the validation runs against the constraint, i try to make cases for the constraint that touch the different paths throuugh the constraint.

In the parent issue there were indeed 2 constraints that needed tests, and since these validation issues have been broken down a lot, i opted to create 2 issues for those contraints. But to test, you need the contraint also so it is implemented here.

Now sure what test you are missing, isnt NoEntitiesExistYetWithHigherCardinalityTest a test for this contraint? Seems reasonable to test like that. If you recon that is not enough coverage please let me know what you want more.

Shoudlnt the parameter be typed? Its new and seems to be an ?orderinterface