Account created on 5 November 2008, over 16 years ago
#

Merge Requests

More

Recent comments

🇫🇮Finland sokru

I guess it might be related by some faulty Search API configuration or source data issue, but on the other hand search_api_opensearch module has this change committed since day 0 https://git.drupalcode.org/project/search_api_opensearch/-/blob/3.x/src/...

🇫🇮Finland sokru

The issue was persistent on large Drupal 10.3.x project, however I'm not able to reproduce it anymore, so closing this. The original issue might need to be resolved on Drush or PHP memory configurations level.

🇫🇮Finland sokru

Would be benefical to have some tests for drush commands, but I'd leave it for another issue.

🇫🇮Finland sokru

sokru made their first commit to this issue’s fork.

🇫🇮Finland sokru

I've attached a tiny patch for core Claro theme to fix the issue, but IMHO it should be handled in core with BC etc.

So unless @dhruv.mittal has some better idea how to fix the issue, this I'll mark this Postponed on 🌱 Propose the Preview feature as an experimental Core feature Postponed . Feel free to create a corresponding core issue.

🇫🇮Finland sokru

Its read on /admin/config/content/same-page-preview on page load and stored to localStorage. https://git.drupalcode.org/project/same_page_preview/-/blob/2.1.x/js/adm...

I'd say the interpretation on issue summary is wrong and we should limit the scope of this issue just for the label copy.

Whatever label we choose it should be reflected to config schema on related issue.

🇫🇮Finland sokru

Added related (child) issue.

🇫🇮Finland sokru

+1 for RTBC.
I've created a separate issue I found when testing this issue 🐛 View mode does not persist on validation errors Active .

🇫🇮Finland sokru

@niharika.s Thanks for your effort, but did you test your patch with linked module? It needs more complete refactoring to work.

I've attached a patch for work-a-round for domain-module, but I'd say the fix needs to be worked on same_page_preview module.

🇫🇮Finland sokru

Added the upgrade path, tested manually on project with config:inspect command that configurations are valid now.

🇫🇮Finland sokru

On issue 📌 Add index to AlterSettingsEvent for backward compatibility Fixed there's some example code using EventSubscriberInterface, would that help you?
We probably should update the documentation ( https://www.drupal.org/docs/contributed-modules/elasticsearch-connector ) for better crafted examples.

🇫🇮Finland sokru

Created a MR that fixes the issue, but this module would greatly benefit for tests for this feature.

🇫🇮Finland sokru

This can be reproduced easily when creating a new content with Drupal core 11.x and 2.1.4/latest dev commit. I tried reverting 🐛 Preview only works when editing content, not creating Fixed but it didn't help. Attached a screen recording.

🇫🇮Finland sokru

The patch contains proof-of-concept of the fix, it should be rather safe change.

🇫🇮Finland sokru

Thanks for the great module!
I didn't create a separate service to do requirement check like the main module is doing.
The submodule would also benefit for READMD.md or INSTALL.md help file, eg. exact composer steps how to download latest supported version.
Seems like https://github.com/josdejong/jsoneditor stopped including dist/ to codebase somewhere on 6.x.x.

🇫🇮Finland sokru

sokru made their first commit to this issue’s fork.

🇫🇮Finland sokru

This still needs work for issue mentioned on #12 (and #16). So it needs steps to reproduce, fix for that and tests to make sure it won't get regression.

🇫🇮Finland sokru

This needs tests. I'd recommend adding tests from https://www.drupal.org/files/issues/2023-10-05/2954352-diff-show-local-t...
and separate test for issue reported on #12.

🇫🇮Finland sokru

Looks awesome! Only thing I wonder if we should mark the template deprecated [#3385127] ?

No contrib modules seem to alter the template, but some custom projects might.

🇫🇮Finland sokru

For us the MR on related issue 🐛 AJAX MessageCommand markup and styling differs from Theme default Active does not fix the issue. We have a custom module with form_alter that places custom inline status message on specific place. After the 📌 Use MessagesCommand in BigPipe to remove special casing of the messages placeholder Fixed all status messages are rendered on same place:

This issue can be reproduced easily with core (11x.) by creating a custom module with following code and setting the site to maintenance mode and going to "/node/add/article".

<?php
use Drupal\Core\Form\FormStateInterface;

function MY_MODULE_form_node_form_alter(&$form, FormStateInterface $form_state, $form_id) {
  // Some business logic.
  $form['field_example'] = [
    '#theme' => 'status_messages',
    '#status_headings' => [
      'error' => t('Error message'),
    ],
    '#include_fallback' => TRUE,
    '#message_list' => [
      'error' => [
        t('Failed to connect the API service, please try again later.'),
      ],
    ],
  ];
🇫🇮Finland sokru

Updated the issue summary for the issues that I found that should be finished before the beta/RC release. Maintainers might disagree with the list, but I'm sure they'd appreciate any help to complete any issue on issue tracker.

On general level the module has a lot of code inherited from Drupal 7 days. Also many issues would need someone to validate if the issue is still present and provide steps how to reproduce it.

🇫🇮Finland sokru

Unfortunately this breaks sorting when "Allow multiple selections" is selected. I'd recommend reverting the commit #54b5ef33 and adding the solution with tests.

🇫🇮Finland sokru

sokru made their first commit to this issue’s fork.

🇫🇮Finland sokru

I'll leave to maintainers to decide if they first want to complete the 🌱 Code modernisation Active .

🇫🇮Finland sokru

Needs work for field description. The failing tests are in process on several issues.

🇫🇮Finland sokru

sokru made their first commit to this issue’s fork.

🇫🇮Finland sokru

The tests on MR pass on Drupal 10, but fails on Drupal 11. Maintainers could either close this as duplicate for 📌 Drupal 11 compatibility Needs work , or merge this and handle Drupal 11 related tests on related issue.

🇫🇮Finland sokru

sokru made their first commit to this issue’s fork.

🇫🇮Finland sokru

The failing tests seems unrelated.

🇫🇮Finland sokru

I gave some thought on documentation page update, but I'd say its better to leave as simple as possible. The documentation page gives the instructions for "--help" flag for quick-start command, and that shows the instructions for all the options, including the password option.

🇫🇮Finland sokru

Required quite many restarts until the the unrelated test failures got passed status.

🇫🇮Finland sokru

Great work here @mparker17! I'll look into this in upcoming weeks.

🇫🇮Finland sokru

I fully support the idea, only thing I wonder is that many still uses outdated Drupal ( https://www.drupal.org/project/usage/drupal ) and its usually easier to upgrade contrib-modules before doing the core update. But maybe we leave 8.x-7.x branch for that process...

🇫🇮Finland sokru

sokru made their first commit to this issue’s fork.

🇫🇮Finland sokru

@murrow, you're correct there's currently no support for proxy connection.

🇫🇮Finland sokru

I think all threads should be resolved now.

🇫🇮Finland sokru

I'd recommend merging 📌 Add Gitlab CI to siteimprove module Active first, so we get extra assurance the this module is compatible with Drupal 11. So the now failing job "phpstan (next major)", should pass.

🇫🇮Finland sokru

I tried to keep the changes as minimal as possible, Eslint, Stylelint and CSPell issues should be handled on separate issue.

🇫🇮Finland sokru

I believe the correct status now is "Fixed".

🇫🇮Finland sokru

I believe the correct status is now "Fixed"

🇫🇮Finland sokru

@arunmark thanks for the merge request, however this still needs the upgrade path and the config updates, similar to what has been done on [PP-1] Make views exposed filter operator labels configurable Needs work .

🇫🇮Finland sokru

re: #46
Asking users to go to separate form to resolve this issue does not help the command-line use cases (CI pipelines, syncing database from different environment, etc.).

IMHO automatically creating the missing translation directory gives the best DX/UX. We could add some message to inform about autocreation.

🇫🇮Finland sokru

Related issue has tests and upgrade path that this is missing. On Views admin UI the patches from this issue uses the same label "Operator label" as [PP-1] Make views exposed filter operator labels configurable Needs work , either of them would need a change the label to avoid confusion.

🇫🇮Finland sokru

I don't think its postponed by anything, but needs a reroll.

🇫🇮Finland sokru

Removed the duplicate function that @quietone found, setting back to Needs review.

🇫🇮Finland sokru

Committed to 8.0.x, not sure if I find a time to fix this also on 8.x-7.x.

🇫🇮Finland sokru

I wrote the steps to reproduce the issue and created draft change record.

🇫🇮Finland sokru

@gapple Caching the nonce might be acceptable for someone, but tools like https://csp-evaluator.withgoogle.com/ or https://github.com/google/csp-evaluator for offline usage, will mark it as a violation: "This CSP contains static nonces. Static nonces allow attackers to bypass the CSP since they know the nonce value. Nonces should be cryptographically random."

Quote from https://content-security-policy.com/nonce/ under subtitle "Cons of using a Nonce vs a Hash":

The nonce needs to be generated dynamically, and must be different for each request

Netlify platform has CSP plugin, enabling it will result a random nonce value on each request. You can try by running curl -I https://app.netlify.com/ |grep 'content-security-policy' multiple times and nonce is different on each request.

🇫🇮Finland sokru

I thought I would be working on the failing tests, but after some reading on the subject I think the nonce should not be handled on application or backend level, becuase like @gapple mentioned on #2:

A nonce must be randomly generated per-response, not just per-site.

that's going to require disabling all the caches. This article describes an approach, where backend just provide a placeholder for nonce and proxy replaces the placeholder for each request.

Therefore I suggest we either close this issue as "won't fix" or re-title this for suggestion number 2 from issue summary "Implement a sha-X hash".

🇫🇮Finland sokru

+1 for adding this feature, so it would make easier to land the parent issue.
Left suggestion also to add H6 -level element.

I assume #18 was missing a cache clearance.

🇫🇮Finland sokru

I went through the remaining tasks and all but last issue was not done. I created a MR for remaining task.

🇫🇮Finland sokru

I assume usability review could think following issues:
- Position of lossless checkbox on form.
- Need for lossless checkbox on form. Since the IMG_WEBP_LOSSLESS equals to 101 value ( see: https://php.watch/versions/8.1/gd-webp-lossless). We could remove the checkbox and treat the 100 selection same as lossless selection (This is how Intervention/image does it)

🇫🇮Finland sokru

sokru made their first commit to this issue’s fork.

🇫🇮Finland sokru

This does not fix the Eslint issues, because that would require reroll's to existing Javascript issues.

🇫🇮Finland sokru

Drupal 10.2. is supported at least until December 2024, also given that almost 50% of Drupal 8+ installations are running on outdated core I think this issue should either be postponed or at least require 10.3 version on composer.json.

🇫🇮Finland sokru

Addressed the latest feedback. However I didn't modify the change record, since I could not think up of any useful example and I did not find any other change record which would give an examples of new services. Given the age of this issue I think the CR can be updated later if someone comes up with any useful examples.

🇫🇮Finland sokru

Rebased the MR and fixed few coding standards related issues. A bit unsure about the test coverage, since the tests seem to pass even if I mix the contents of test/expectations/read_only_* directories.

🇫🇮Finland sokru

Self RTBC'ing since this is really simple fix. Maintainers feel free to change the status if you require tests or something.

🇫🇮Finland sokru

we should consider is what if someone wnats to compare current revision with a revision on a 2nd page, will that still be possible.

Quoting @berdir from #43 🐛 Node revisions tab have "Current Version" on every Page Needs work

drupal core doesn't allow you to actually compare versions, that's diff.module which mostly replaces the current core functionality, as there's not really another way. With core, you can just manually compare by viewing the different revisions in separate tabs, that could be done across multiple pages.

I assume we don't want to include Diff module into core on this issue, so setting back to Needs review. Diff module uses routeSubscriber so changes in this MR does not have effect on Diff module.

🇫🇮Finland sokru

sokru changed the visibility of the branch 3021671-node-revisions-tab to hidden.

🇫🇮Finland sokru

Updated the issue summary and rebased the MR to 11.x branch. I agree with #45 that doing more complete UX renew should be done on separate issue so we could fix this 6 years old issue first.

🇫🇮Finland sokru

sokru made their first commit to this issue’s fork.

🇫🇮Finland sokru

The solution is from referenced issue. Elastcsearch-php library has open issue to make the client report better on connection errors. Not sure if Opensearch-php library has yet a open issue.

Production build 0.71.5 2024