🇫🇮Finland @sokru

sokru → created an issue.

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

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

@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.

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".

+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.

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

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

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)

Feedback has been addressed.

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

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

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.

sokru → created an issue.

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.

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.

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

sokru → created an issue.

sokru → changed the visibility of the branch 3.x to hidden.

sokru → changed the visibility of the branch 8.x-2.x to hidden.

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

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

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.

Resolved the threads created by @alexpott and created a follow-up issue for Navigation module 📌 [PP-1] Navigation module should honor admin language on frontpage and node route Postponed .

sokru → created an issue.

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

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.

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

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.

sokru → created an issue.

Added the tests, reverting the changes to src/SearchAPI/BackendClient.php will make the test fail.

I added one @todo comment with link to upstream issue (https://github.com/elastic/elasticsearch-php/issues/1308) getting that done would help providing more informative error messages. Currently the wrong port will produce an error "Elastic\Transport\Exception\NoNodeAvailableException: No alive nodes. All the 1 nodes seem to be down", which is misleading.

The title of this issue could be better.

@hswong3i please revert your last commit, MR's should not combine multiple issues into one MR.

The MR handles the error, but wonder if this needs a Functional test, something like https://git.drupalcode.org/project/search_api/-/blob/8.x-1.x/tests/src/F...

Actually still applies to 8.0.x-dev (and search_api_opensearch).

Looks good to me, only thing I was wondering that we hardcode the fuzziness value here to "auto" and someone might expect to get the setting from here: https://git.drupalcode.org/project/elasticsearch_connector/-/blob/8.0.x/...

@mile23: Latest release 3.0.0-beta1 is from 2022, the duplicate issue 📌 PHP 8.2 compatibility Fixed was committed to 3.0.x branch on 2023. So you should either use dev-branch or pick the patch for 3.0.0-beta1 from mentioned issue.

The maintainers should create a new release for Facets

@catch Threads are open, because only maintainer or @quietone is able to resolve them. One suggestion was applied and another has been done at a6bcfa69.

Added new issue 🐛 Upgrade path for full_date data type Active and lowered the priority of 🐛 Error with facets "Expected [START_OBJECT] under [filter], but got a [START_ARRAY]..." Needs review since cannot reproduce it.

@mparker17: I would greatly appreciate if you could update the change record.

I added the deprecations for services, I think we should not add deprecations to classes, since its lot of work and it makes extra work to reroll any potential patches for 8.x-7.x branch.

Wonder if there's any other deprecation we should (or could) add.

The title and issue summary should be also updated to reflect what we have done.

sokru → created an issue.

The tests have never passed on, not even during the DrupalCI times, so arguable if this issue should be on bug or task category. Here's the link for previous attempt to fix the tests https://git.drupalcode.org/project/elasticsearch_connector/-/merge_reque...

And like I said if someone is able to make the tests pass, I'm happy to commit the MR.

Thanks @kevinvb for working on this issue, it still needs more work to complete the PHPUnit tests on 8.x-7.x. In the past I have spent several days trying to fix the PHPUnit tests on 8.x-7.x branch, but its rather complicated. I'm happy to commit the MR once the PHPunit job passes.

On general level, the current maintainers hope that people would swift to use 8.0.x branch, where the test coverage is better and CI pipelines are green.

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

sokru → created an issue.

+1

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

I did my best to update the issue summary, tried to split the remaining tasks.
IMHO this should include an upgrade path for views block_content that is on route /admin/content/block.

This needs work for translations: Block content has translation, but translation unpublished, the source language is rendered. Something like this fixed the issue, but probably would better to be done at BlockContentBlock::getEntity()

--- a/core/modules/block_content/src/Plugin/Block/BlockContentBlock.php
+++ b/core/modules/block_content/src/Plugin/Block/BlockContentBlock.php
@@ -185,7 +185,9 @@ protected function blockAccess(AccountInterface $account) {
    * {@inheritdoc}
    */
   public function build() {
-    if ($block = $this->getEntity()) {
+    $currentLangcode = \Drupal::languageManager()->getCurrentLanguage()->getId();
+    $block = $this->getEntity();
+    if ($block && $block->language()->getId() === $currentLangcode) {

- Added upgrade path and tests for it.
- Changed the UI facing texts from "WEBP" to "WebP".
- Since Drupal 11 requires PHP 8.3, there's no need to check for support of IMG_WEBP_LOSSLESS
- Hide the outdated patches.

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

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

I was not able to reproduce this issue on latest version (2.1.1) with Drupal 11 and Claro admin theme.
The patches here are basically a revert for #3001262: Integrate focal point with media_library, which is now in core → . On that issue people are complaining that the feature does not work.
Also if I add some debug messages on templates/focal-point-media-library-image-widget.html.twig file it does not appear anywhere, so gives an confirmation that template file is obsolete.

Unfortunately I was not able to reproduce the issue with steps mentioned on the issue summary.
I was using the page cache + Drupal 11 + focal_point 2.1.1 (with 🐛 Preview results in Error: Call to a member function getDefinitions() on null RTBC ) + Crop API 2.4.
So even without clicking preview link the page cache got MISS and the image got updated also for anonymous users.

I'm marking this as postponed until there's steps to reproduce on vanilla Drupal installation.

Regarding to #27/#35 about webp support, core has supported webp -format out-of-box for quite long time ( #2340699: Let GDToolkit support WEBP image format → ), so if this issue is still valid, we should make sure it works with webp format.

I tried to replicate the issue with latest 8.0.x branch, I created a view search page and added two facets (taxonomy term and node-id) but unfortunately I was not able to get any error message. Would you be able to replicate the error with fresh site and latest version of Elasticsearch connector 8.0.x module?

Thanks, committed.

Sorry for the noise. I was trying to target this MR against 2.0.x where the tests are now running, but seems like its only MR author or project admin who should do that.

sokru → changed the visibility of the branch 2.0.x to hidden.

sokru → changed the visibility of the branch 8.x-1.x to hidden.

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

The tests pass now, seems like "_PHPUNIT_CONCURRENT" was causing issues, increasing the memory resources for Elasticsearch partially helped on the issue.

One question left for @mparker17, otherwise this is good to go.

I added tests for FileEncodingConstraint. Instead of list I converted $encoding into an array, IMO it makes the code cleaner: no need for implode or preg_* that FileExtensionConstraint is using.

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

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

I think other threads are solved, but still open to suggestions if the FileEncoding constraint $encoding should be list of encodings.

I cleaned up the code a bit and used same terminology as media_entity_file_replace module is using.

Addresses the usability review recommendation #96.3 by not requiring the user to use same file name as the existing file. However requires the mime type to be same as existing file.

#96.1 and #96.2 might not be feasible since EntityBase does not offer "replace" function.
#96.4: That would require reliable entity-usage system, so I'd suggest creating a separate issue for it and postponing it by one of the entity usage issues.

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

Removed the outdated code

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

The upgrade path didn't caught this, should create a new issue for providing the upgrade path.
Using the "string" plugin provides following date

1919-01-10T10:10:10

Which I hope is sufficient.

Added the recommendation to MR. I don't see any draft workflow on wiki page, so I assume updating the documentation should be done only after MR has been merged.

Added the change notice, all the threads on MR should be resolved, so setting the status "Needs review".

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

Should be committed.

PHPCS and CSPell issues could be easily fixed on the same merge request, but leaving up to maintainers to decide if that is required on the issue scope.

You need to change your root composer.json "minimum-stability" to "beta" in order to install this module.

The pipelines are green, so self RTBC'ing this. This would also make module loadable for Drupal 11 projects.

sokru → created an issue.

Based how webform handles this, we should mention the version just once.

Unfortunately this didn't seem to be working, see related issue.

Resolved the PHPCS issues, caused by other issues merged after PHPCS issue.

I've tested this manually to be working.

I created a separate issue for resolving PHPStan issues 📌 Resolve PHPHStan issues Active .

sokru → created an issue.

Resolved the issue mentioned on #14 by changing the option to more generic.
I also refactored the tests to include test for actual merging.
Leaving up to maintainers if they wish tests also for media entities. IMHO it feels a bit of repetition (and maintenance burden) to include also tests for media entities.

sokru → changed the visibility of the branch 8.x-1.x to hidden.

sokru → changed the visibility of the branch 3266357-remove-entity-doesnt to hidden.

Converted the patch from #6 into merge request and made minor coding standard fixes. Tested manually to be working, although I leave it up to maintainers if they require tests for this.

I was not able to replicate this with current core admin theme (Claro).

I'd assume the authors would prefer merge request, also make the submodule Drupal 11 ready by using:

core_version_requirement: ^10 || ^11

otherwise it looks good to go. Another thing to consider is to refactor this module so that it would not require submodule for each entity types (Menu, Block, Media etc.).

Tested to be working, even the drush command succeeded on Drupal 11.

Please commit and make a new release. I'd recommend dropping the support for Drupal 8 + 9 on 2.x branch.

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

The feature is provided by ext-persist extension (https://github.com/mar10/fancytree/wiki/ExtPersist)

See also 📌 Consider jstree as alternative for jquery.fancytree Active and for implementation with fancytree: #2902828: Add fancytree's Drag-and-drop support (native HTML5) - ext-dnd5 →

sokru → created an issue.

Keyboard navigation is provided with https://github.com/mar10/fancytree/wiki/ExtTable extension, but maintainers should consider also other options (e.g. https://www.npmjs.com/package/jstree, half the size, no jquery-ui dependency).

sokru → created an issue.