Account created on 15 June 2009, about 15 years ago
#

Merge Requests

More

Recent comments

πŸ‡ΊπŸ‡ΈUnited States nicxvan

I think as long as there is a way to generate it locally just like https://www.drupal.org/project/drupal/issues/3403649#comment-15331668 πŸ“Œ Rework database update tests so we don't have to ship database dumps in git Active suggests it should be fine.

I also think this is much lower stakes and easier to tinker with than full DB dump generation.

We can also just postpone this and πŸ“Œ Decompress files for update_test_new_module Active

πŸ‡ΊπŸ‡ΈUnited States nicxvan

Thank you. This issue was more about evaluating the code compatibility with Drush.

I think the next version is going to drop drupal 9 support, if drush minimum is 12 then there might be some code in the module itself we can clean up.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

Setting needs review for the two approaches in the IS.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

nicxvan β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

Thanks guys.

This is in the dev release. I'll do a new release in a bit.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

Ah, I knew I was missing something obvious on the tests.

I already filed an issue against drupal phpstan for mglaman.

Tests are now passing thanks!

πŸ‡ΊπŸ‡ΈUnited States nicxvan

What version of drush are you running?

πŸ‡ΊπŸ‡ΈUnited States nicxvan

Is there any chance I can just deprecate the method getConfigTarball and add getConfigLocation.

Creating the new class and swapping things over should be straightforward, but almost every test using it is breaking now even though it's an exact copy of the previous version of the MR. The only change is I had to add a default theme.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

nicxvan β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

nicxvan β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

Adding some detail to the Issue summary from slack.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

Ah I missed the test only patch, that's why I was confused. Thanks for clarifying.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

@ pooja_sharma

Can you clarify what you are splitting into the fix code vs test code?

πŸ‡ΊπŸ‡ΈUnited States nicxvan

I addressed your second comment.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

Committed the suggestion and answered your question.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

I would like to continue maintaining this project.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

I understand that the security team has a large load, but in cases like this a blanket polity of marking these releases as security would help the community at large and I think improve the secure reputation Drupal enjoys.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

Also if you're providing a patch for an issue with a merge request make sure you hide it so that work is focused in the merge request.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

nicxvan β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

I set string|false since https://www.php.net/manual/en/domdocument.savexml.php shows that, let me know if that should be handled differently.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

I'll take a look

πŸ‡ΊπŸ‡ΈUnited States nicxvan

I'm no longer getting this error, I added some more detail in πŸ“Œ What is the proper way to inject the cache backend? Needs review with what I am experiencing now.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

This along with πŸ› ConnectionStub throws error when fetchAll is called on execute. Fixed mostly solved my issue.

In this service I'm collating data from some fields on a taxonomy term and flattening them in an array I can cache and access.

In the test I'm creating the vocabulary, then a test term like this:

    $vocabulary = TestHelpers::saveEntity('taxonomy_vocabulary', [
      'vid' => 'part_color',
      'name' => 'Part Color',
    ]);

    $term = TestHelpers::saveEntity('taxonomy_term', [
      'vid' => $vocabulary->id(),
      'name' => 'A1',
      'parent' => 0,
      'field_color_code' => '23',
      'field_thirdparty_id' => 'thirdpartyId',
      'field_transparent' => TRUE,
    ]);

    TestHelpers::service('database');
    TestHelpers::service('cache.data');
    $customColors = TestHelpers::service('custom_colors.get_colors', customColors::class); 
    $colors = $customColors->getColors();
    print_r($colors);
    $color = reset($colors);
    $this->assertCount(1, $color['cid']);

However in the code I'm doing a direct db call to join the fields since it's a lot more efficient in this case.

If I understand it correctly I need to create the three additional fields expected field_color_code, field_thirdparty_id and field_transparent. Is that correct? Is there a test helper for creating mock fields like that?

πŸ‡ΊπŸ‡ΈUnited States nicxvan

One final comment for now, I didn't check if I could get failures for the three on HEAD with the same technique, so the tests could have been broken already and not affected by this.

  • linkExists
  • assertEscaped
  • responseContains

I would think that would make fixing those a follow up too, but I can't check that right now.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

Just to expand linkExists is covered by testPipeCharInLocator:

public function testPipeCharInLocator(): void {
    $this->visit('/test-pipe-char', '<a href="http://example.com">foo|bar|baz</a>');
    $this->assertSession()->linkExists('foo|bar|baz');
    $this->addToAssertionCount(1);
  }

Removing the assertion should fail the test if I understand it correctly but does not.

public function linkExists($label, $index = 0, $message = '') {
    $message = ($message ? $message : strtr('Link with label %label not found.', ['%label' => $label]));
    $links = $this->session->getPage()->findAll('named', ['link', $label]);
    $this->assert(!empty($links[$index]), $message);
  }
πŸ‡ΊπŸ‡ΈUnited States nicxvan

Well I decided to do an audit since I was curious and there are some gaps. The second list likely needs addressing here. The third list in a followup.

I decided to check locally actually. I wanted to learn more about WebAssert anyway.

I cloned your branch and ran the WebAssertTest. All green!
In Web Assert I then would break the function by commenting out the assert or otherwise breaking the logic.

I have confirmed coverage for:

  • responseHeaderExists
  • responseHeaderDoesNotExist
  • pageTextMatchesCount
  • buttonNotExists
  • linkExistsExact
  • linkNotExistsExact
  • linkByHrefExists
  • linkByHrefExistsExact
  • linkByHrefNotExists
  • linkByHrefNotExistsExact
  • pageTextContainsOnce
  • pageContainsNoDuplicateId
  • addressNotEquals
  • elementTextEquals

WebAssertTest claims coverage, but I cannot trigger a failure

  • buttonExists
  • selectExists
  • optionExists
  • buildXPathQuery
  • linkExists
  • assertEscaped
  • responseContains

WebAssertTest does not claim coverage, but probably should cover these.

  • optionNotExists
  • titleEquals
  • assert
  • fieldDisabled
  • fieldEnabled
  • hiddenFieldExists
  • hiddenFieldNotExists
  • hiddenFieldValueEquals
  • hiddenFieldValueNotEquals
  • statusMessageExists
  • statusMessageNotExists
  • statusMessageContains
  • statusMessageNotContains
  • buildStatusMessageSelector
  • responseHeaderEquals
  • pageTextContains
  • fieldValueEquals
πŸ‡ΊπŸ‡ΈUnited States nicxvan

This is a massive improvement for this test!

Looks good to me generally, but there was a comment from you in slack in response to a question that I think should be recorded here and addressed.

https://drupal.slack.com/archives/C1BMUQ9U6/p1718924045498369
mstrelan
3 hours ago
so the thing is that this test is not testing that the escaping thing works correctly, its testing that the assert methods work correctly

So in reviewing this I realize this is kind a meta-test. It's a test testing a test class. Normally when there is a bugfix or feature change you need a test only job to prove there is actual coverage. That won't quite work here cause the only thing changed was the test itself so test only would be the same change!

I wonder if we need another branch with these changes but the WebAssert class intentionally broke to prove this version still has the same coverage with the same rules as test only, it should fail.

That being said it looks good to me and consistent, I just don't know if we need the equivalent of the test only job in order to verify.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

After reading the comment from @pameeela I don't think the country setting needs a release note.

@smustgrave that is a good point, I think since we added config new config we didn't hit the circular issue @dww hit here: https://www.drupal.org/project/drupal/issues/2640994#comment-15597379 πŸ› Fix label token replacement for views entity reference arguments RTBC so these probably can stay in highlights unless someone disagrees.

I think my confusion stemmed from this comment: https://www.drupal.org/project/drupal/issues/3333418#comment-14886507 πŸ› Stable9 accessibility update: Pager h4 causes accessibility flag on many pages RTBC
and since that issue was a follow up I assumed the parent needed release notes too, but based on the docs I think they fit better under highlights.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

@pamneela good point actually. Existing sites already have the setting. New installs of core don't need it. And for people that need it after installing they have the change record. I've untagged.

Sorry for the confusion.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

I tried a couple more and they seem to work like <table>

That seems to confirm, details and custom html elements do not interact properly.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

I changed the SDC from details to a div and the SDC worked, I suspect this is the reason that the native web components don't work either. I suspect the library looking for the attributes is expecting them on a specific element and if it's not the correct element the JS cannot place the toolbar for the edit.

I tried

and those work.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

Ok I have tested again and there is still some strangeness. I took the NWCs out of the equation and just used SDCs

It works better but some key things are still broken.

I cannot edit the components once I have added them.

I have confirmed by switch back to olivero and editing / adding new components.

In the custom theme I can add new components, but I cannot edit them, the ui is missing.
I have extensive reproduction steps in the issue summary.

It renders properly on the front end:

<details data-component-id="mercury_theme:accordion">
  <summary>
            <div class="field field--name-field-title field--type-string field--label-hidden field__item">Test accordion 2</div>
   </summary>
            <div class="clearfix text-formatted field field--name-field-content field--type-text-long field--label-hidden field__item"><p><strong>Content</strong> 2</p></div>
</details>

However this is how it renders in the edit side:

<details data-uuid="3315e5dc-9a09-47f5-a39f-e9b33ccc25af" data-type="accordion" data-lpb-ui-id="3315e5dc-9a09-47f5-a39f-e9b33ccc25af" class="js-lpb-component is-mercury-edit-mode" data-has-js-ui-element="" data-component-id="mercury_theme:accordion" data-once="lpb-ui-elements me-prevent-focus" tabindex="-1"><a class="lpb-btn--add before use-postmessage js-lpb-ui" data-dialog-type="dialog" data-dialog-options="{&quot;target&quot;:&quot;lpb-dialog-48449700d5b1ad618feac1cc7dfd19d9&quot;,&quot;width&quot;:&quot;fit-content&quot;,&quot;height&quot;:&quot;fit-content&quot;,&quot;drupalAutoButtons&quot;:false,&quot;dialogClass&quot;:&quot;lpb-dialog&quot;,&quot;resizable&quot;:true}" href="/mercury-editor/48449700d5b1ad618feac1cc7dfd19d9/choose-component?sibling_uuid=3315e5dc-9a09-47f5-a39f-e9b33ccc25af&amp;parent_uuid=adf03d4c-4094-4d87-9ba7-1d0baa0eb9d0&amp;region=content&amp;placement=before" data-once="me-msg-broadcaster me-stop-iframed-links me-prevent-focus" target="_parent"><span class="visually-hidden">Choose component</span></a><div class="lpb-controls js-lpb-ui" style="null">
    <a href="#move" class="lpb-drag lpb-tooltip--hover lpb-tooltip--focus" aria-describedy="lpb-controls--2--tip" data-once="me-stop-iframed-links me-prevent-focus">Drag</a><span class="lpb-tooltiptext" id="lpb-controls--2--tip">Drag or click and use arrow keys to move. <br>Press Return or Tab when finished.</span>
<span class="lpb-controls-label">Accordion</span>
<a href="#move-up" class="lpb-up" title="Move up" tabindex="-1" data-once="me-stop-iframed-links me-prevent-focus">Move up</a><a href="#move-down" class="lpb-down" title="Move down" tabindex="-1" data-once="me-stop-iframed-links me-prevent-focus">Move down</a><a href="/mercury-editor/48449700d5b1ad618feac1cc7dfd19d9/edit/3315e5dc-9a09-47f5-a39f-e9b33ccc25af" class="lpb-edit use-postmessage" data-dialog-type="dialog" data-dialog-options="{&quot;width&quot;:&quot;fit-content&quot;,&quot;height&quot;:&quot;fit-content&quot;,&quot;drupalAutoButtons&quot;:false,&quot;dialogClass&quot;:&quot;lpb-dialog&quot;,&quot;resizable&quot;:true,&quot;target&quot;:&quot;lpb-dialog-48449700d5b1ad618feac1cc7dfd19d9&quot;}" title="Edit Accordion" data-once="me-msg-broadcaster me-stop-iframed-links me-prevent-focus">Edit</a><a href="/layout-paragraphs-builder/48449700d5b1ad618feac1cc7dfd19d9/duplicate/3315e5dc-9a09-47f5-a39f-e9b33ccc25af" class="lpb-duplicate use-ajax" title="Duplicate" data-once="ajax me-stop-iframed-links me-prevent-focus">Duplicate</a><a href="/mercury-editor/48449700d5b1ad618feac1cc7dfd19d9/delete/3315e5dc-9a09-47f5-a39f-e9b33ccc25af" class="lpb-delete use-postmessage" data-dialog-type="dialog" data-dialog-options="{&quot;width&quot;:&quot;fit-content&quot;,&quot;height&quot;:&quot;fit-content&quot;,&quot;drupalAutoButtons&quot;:false,&quot;dialogClass&quot;:&quot;lpb-dialog&quot;,&quot;resizable&quot;:true,&quot;target&quot;:&quot;lpb-dialog-48449700d5b1ad618feac1cc7dfd19d9&quot;}" title="Delete Accordion" data-once="me-msg-broadcaster me-stop-iframed-links me-prevent-focus">Delete</a>
  </div>
  <summary>
            <div data-sync-changes="paragraph/3315e5dc-9a09-47f5-a39f-e9b33ccc25af/field_title" class="field field--name-field-title field--type-string field--label-hidden field__item">testt</div>
      </summary>
  
            <div data-sync-changes="paragraph/3315e5dc-9a09-47f5-a39f-e9b33ccc25af/field_content" class="clearfix text-formatted field field--name-field-content field--type-text-long field--label-hidden field__item"><p>tst</p></div>
      
<a class="lpb-btn--add after use-postmessage js-lpb-ui" data-dialog-type="dialog" data-dialog-options="{&quot;target&quot;:&quot;lpb-dialog-48449700d5b1ad618feac1cc7dfd19d9&quot;,&quot;width&quot;:&quot;fit-content&quot;,&quot;height&quot;:&quot;fit-content&quot;,&quot;drupalAutoButtons&quot;:false,&quot;dialogClass&quot;:&quot;lpb-dialog&quot;,&quot;resizable&quot;:true}" href="/mercury-editor/48449700d5b1ad618feac1cc7dfd19d9/choose-component?sibling_uuid=3315e5dc-9a09-47f5-a39f-e9b33ccc25af&amp;parent_uuid=adf03d4c-4094-4d87-9ba7-1d0baa0eb9d0&amp;region=content&amp;placement=after" data-once="me-msg-broadcaster me-stop-iframed-links me-prevent-focus" target="_parent"><span class="visually-hidden">Choose component</span></a></details>
πŸ‡ΊπŸ‡ΈUnited States nicxvan

In that case the pager ones are likely highlights since they are accessibility improvements (They were split up since one of them was for stable9)
πŸ› Pager h4 causes accessibility flag on many pages Fixed
πŸ› Stable9 accessibility update: Pager h4 causes accessibility flag on many pages RTBC
I've retagged those two already.

The country default in the installer is likely disruptive if someone is expecting that default to be set. It was not used in core and low usage in contrib, but likely qualifies for release notes:
πŸ“Œ Remove country setting from the installer Fixed

πŸ‡ΊπŸ‡ΈUnited States nicxvan

I will double check but you mentioned that on the show so I was careful to ensure I was passing that into the sdc and printing out at the root.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

I think that's a valid question, but since this is a security issue should that be a followup since this is just exporting the tars as is?

The only config change I did was add a missing newline so the validation would work.

I'm not sure how to scope security issues.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

I agree that @mindaugasd's quote seems right.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

Here is my attempt that might be worth working into the first part of wouters_f's description.

The AI module allows you to combine several different AI services into a workflow chain that matches your needs. You can specify the order services are used and what triggers the chain to begin.

It might be worth mentioning that the configuration occurs on the field level.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

Also πŸ› Stable9 accessibility update: Pager h4 causes accessibility flag on many pages RTBC was already tagged, but I don't see it in the release notes.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

I tagged two more issues:

πŸ› Pager h4 causes accessibility flag on many pages Fixed
πŸ“Œ Remove country setting from the installer Fixed

Re reading this issue summary they probably belong in the highlights rather than release notes.

Let me know where they go and I'll retag them.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

Tagging for release note inclusion.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

Tagging for release not inclusion.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

The only place the zip is actually referenced is here:

/**
   * Data provider method for testViaAuthorize().
   *
   * Each of these release URLs has been cached in the setUp() method.
   */
  public static function archiveFileUrlProvider() {
    return [
      'tar.gz' => [
        'url' => 'https://ftp.drupal.org/files/projects/update_test_new_module.tar.gz',
      ],
      'zip' => [
        'url' => 'https://ftp.drupal.org/files/projects/update_test_new_module.zip',
      ],
    ];
  }

I wonder if we can just move these to a contrib module if they are being referenced on d.o anyway.

The tar is referenced above but also in these two places:

$validArchiveFile = __DIR__ . '/../../update_test_new_module/8.x-1.0/update_test_new_module.tar.gz';
    $edit = [
      'files[project_upload]' => $validArchiveFile,
    ];
    $this->drupalGet('admin/modules/install');
    $this->submitForm($edit, 'Continue');
 <release>
  <name>update_test_new_module 8.x-1.1</name>
  <version>8.x-1.1</version>
  <status>published</status>
  <release_link>http://example.com/update_test_new_module-8-x-1-1-release</release_link>
  <download_link>core/modules/update/tests/update_test_new_module/8.x-1.1/update_test_new_module.tar.gz</download_link>
  <date>1300424521</date>
  <terms>
   <term><name>Release type</name><value>New features</value></term>
   <term><name>Release type</name><value>Bug fixes</value></term>
  </terms>
 </release>
πŸ‡ΊπŸ‡ΈUnited States nicxvan

I think this issue should have a test to confirm that there are no zips or tars in the repository to confirm closure of this issue.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

Ok the only reference to this archive file is here:

    // Check to ensure an existing module can't be reinstalled. Also checks that
    // the archive was extracted since we can't know if the module is already
    // installed until after extraction.
    $validArchiveFile = __DIR__ . '/../../aaa_update_test.tar.gz';
    $edit = [
      'files[project_upload]' => $validArchiveFile,
    ];

I think we can just extract it and compress it right before this step as part of the step. It creates a bit of churn on each test, but it does solve the immediate security concern.

There are a lot of references in the code to modules/update/tests/aaa_update_test.module, that file does not exist though so I'm not sure if there is a process I'm missing that extracts the archive first.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

I hid the branch here and moved it to the new child issue that I tidied up and is ready for review now.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

nicxvan β†’ changed the visibility of the branch 3455714-drupal-should-not to hidden.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

I know this was originally created for different reasons, but this is related to a public security issue so I hope it's ok to tag this and mark that issue as the parent.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

I'm going to create some child issues for each type of compressed files since each type requires different approaches,

πŸ‡ΊπŸ‡ΈUnited States nicxvan

aaa_update_test is going to be much more complex since it is explicitly checking the extraction process. We likely need to take a similar process to the db where there is a separate gitlab process to generate them periodically.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

Tests are passing, I'm setting it to needs review for process.

I'm not sure if we need change records for this, I also can see each type of file we address being a separate issue.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

Had to move the poc further along for tests to run.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

@fjgarlin helped, I created the issue with 11.0.x rather than 11.x-dev

πŸ‡ΊπŸ‡ΈUnited States nicxvan

Also not sure why the title of the MR pulled from a cherry picked commit. I used the ui...

πŸ‡ΊπŸ‡ΈUnited States nicxvan

I did a really rough POC for converting the multilingual config from a tar to just a directory that gets copied just to see if things work as expected.

This still needs cleanup, discussion and work.

I'm going to mark as needs review for the approach.

I also had to rebase, not sure how it was out of sync since I just created the branch, but that should be resolved now. I'm not super worried since this is meant more as a poc.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

I wasn't sure which component this should be for since most of it is related to testing.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

Makes perfect sense, let me know if you need something from me!

πŸ‡ΊπŸ‡ΈUnited States nicxvan

Ah, I didn't realize you had to manually start the runs.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

The update bot did not reopen an issue this weekend.

πŸ‡ΊπŸ‡ΈUnited States nicxvan

nicxvan β†’ made their first commit to this issue’s fork.

Production build 0.69.0 2024