🇳🇱Netherlands @bbrala

Fixed in beta5

I think ill just normalize the value to end up with a slash when used. Seems like the best way to make sure things work (also without breaking existing installs).

Just notices the same, ill work on a fix.

Tried reproducing on a fresh install, cant get there.

bbrala → created an issue.

Sounds like a plan to rescope. +1

📌 [PP-1] core.extension should be validatable Postponed landed

BlockFilterTest seems unrelated, since it was annotation only i'm setting back to RTBC

Let me paraphrase @longwave. Instead of triggering the deprecation, just make sure the test expects the casting in those cases right?

yes, wil do now.

Thanks all! I've fixed a few more small things extra :)

Yay, i'd like to opt in, i wanna automate stuff against those issues which is helpfull to get access too then.

This one because ofcourse:
https://www.drupal.org/project/project_analysis →

Also a few smaller projects we have (ill skip jsonapi_extras).

https://www.drupal.org/project/paragraphs_edit →
https://www.drupal.org/project/twig_renderable →
https://www.drupal.org/project/search_api_vragen_ai →
https://www.drupal.org/project/admin_toolbar_entity_version →
https://www.drupal.org/project/admin_toolbar_messages →
https://www.drupal.org/project/admin_toolbar_tasks →

Whee!

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

I'd probably use a small bit of js to keep the state in local storage and apply that.

I wonder if we need to rememeber if the details is open, i can imagine this would help committers in their workflow. Since it saves 33% of the clicks

In regards to the core issue here, i think a no-reply is fine right? I mean, the username is also there to go to the profiles?

One thing i do need to write of me is the fact I'm kinda agreeing with dww and cmlara on the issue as scope, keeping is full conventional commit style.

feat(#3165412): do stuff
...

I did push on the issue in front for scanability, but i think i was wrong in pushing when its non-conform tooling. :(

Yes, ran though, had a small brainfart in regards to scope, but then reread the IS and all is fine. :)

Indeed RTBC

All green after rebase

Rebased. Conflict was with 🐛 Views RSS view mode settings are completely broken Needs work

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

Mixed makes no sense, sorry. Adjusting and merging. Thanks!

System.rss issue has landen and has been removed.

Rss view mode issue has been merged.

Yay! Thank you guys. This was a hard one :)

Implemented the small fix by @alexpott

Setting to RTBC because fix is small

Rebased and resolved the small conflicts. Also fixed phpunit annotation, those needed changing.

It needs a rebase, it has conflicts, so cannot do it in the interface :)

Sorry, one little nit, if awnsered can merge.

Is there anything to write up on how to send the metadata?

We have OliveroPostUpdateTest which at least checks if removal is done properly.

What would you like to see tested? If other settings are kept? Not sure what you are asking more coverage for.

Fixed missing annotation.

The url error was because of the scheduled pipeline having different vars. I've fixed that now. Pipe is running every 5 minutes to check if cache is present. (runtime 38 seconds, dont worry @drumm ;))

bbrala → created an issue.

thanks for the review, updated with the changes. :)

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.