Thanks for the module, looks promising. I tested the issue, but unfortunately the MR didn't fix the issue with 10.3 + PHP 8.2. Still gets the same 400 error with message on log:
Drupal\Core\Http\Exception\CacheableBadRequestHttpException: Invalid nested filtering. The field `vid`, given in the path `vid.meta.drupal_internal__target_id`, does not exist. in Drupal\jsonapi\Context\FieldResolver->resolveInternalEntityQueryPath() (line 297 of /var/www/html/core/modules/jsonapi/src/Context/FieldResolver.php
Rebased and the tests are now green, so setting the status to RTBC.
The project page has button to copy the description from codebase's README.md file so I think it makes sense to also update the file.
Modified the roadmap table to also include the maintenance status of various versions. Wonder if we should make even stronger recommendation for 8.0.x version.
Looks good, I'll update the project page soon.
I'll add https://www.drupal.org/project/search_api_elasticsearch_synonym → to list of modules that extend Elasticsearch Connector. Currently it relies on events provided by 8.x-7.x, so I should work on 📌 Deprecate events and hooks in 8.x-7.x Active and 📌 Add index to AlterSettingsEvent for backward compatibility Active so the upgrade would be easier for modules extending Elasticsearch Connector.
Thanks all! I committed this even there are plans to deprecate "elasticsearch_connector_views" submodule for 8.0.x.
Thanks!
Thanks for this! Requested few changes. I suggest we keep compatibility for drush v11.
Added a subtask 📌 Inform screen readers about paragraph actions (WCAG 4.1.3) Active .
@acbramley could you provide a bit more information about the issue you have faced? I'm not being able to reproduce it on route diff.revisions_diff eg. /node/1/revisions, when selecting also "Show primary tabs" on "Secondary tabs" blocks.
hm, does Safari also ignores must-understand Cache control value? :O
Yes, Safari does ignore must-understand with back-button, Safari behavior is similar to other browser if the "disable cache" is selected on developer toolbar.
Serving responses with Cache-Control: no-store has been the source of wicked UX issues in the past. This needs careful manual evaluation of multiple scenarios across popular browsers. Some scenarios are described in this rather old atlassion blog post.
It would be very beneficial to get these weird UX issues documented with repro steps. On core's /contact/feedback contact form I was not able to find any difference between no-store and must-revalidate, no-cache, using browsers back & forward buttons.
My hunch is that the Cache-Control: no-store response header should be used on specific routes, i.e., on pages which display sensitive / confidential data
Or we could make Drupal more secure by default using no-store and if people need better caching, they could change the response header. Many times even the unpublished node title could contain confidential data.
But I acknowledge there are risks of making this change, so it might be best just to close this issue and introduce a contrib module with eventSubscriber using Symfony's HeaderBag to get a desired Cache-Control header.
@marpker17 Thanks! I'll merge this now so its easier to test upgrade process with custom eventsubscribers. The remaining task is to update change record [#3433223], but I'd say its more related to 🌱 Plan for 8.0.0-alpha release Active , so marking this "Fixed".
sokru → changed the visibility of the branch 1912514-fix-vary-2 to hidden.
I tested @znerol's MR !3505 with DDEV, it works only with HTTPS enabled, like mentioned on OWASP documentation. Tested with Firefox, Chrome, Edge. Unfortunately on Safari the back button showed the cached content.
Wrote on 🐛 Incorrect Cache-Control headers for authenticated users Postponed that we should implement the Cache Control header change on that issue. It will partially fix this issue. On Safari there's a bug https://discussions.apple.com/thread/251817133 and I hope we could narrow down this issue just for Safari.
On Slack @catch suggested moving this to "request processing system" since this is not cache subsystem issue. Cleaned the tags based on that.
The scope of this issue should be making sure the private information is not stored in browser disk. This will resolve the security scanner reports mention on
#5
🐛
Incorrect Cache-Control headers for authenticated users
Postponed
. Changing the Cache-Control header from must-revalidate, no-cache into no-store fixes the
🐛
Using the back button after logging out shows you pages from the authenticated user's session
Needs work
on Firefox, Chrome, Edge, but not on Safari, see https://discussions.apple.com/thread/251817133. I'd suggest leaving that issue to solve issue with Safari.
Should we consider Option 3:Cache-Control: no-store? On linked issue
🐛
Incorrect Cache-Control headers for authenticated users
Postponed
there's patches to implement it with test coverage. OWASP recommends the approach also on its FAQ page https://owasp.org/www-community/OWASP_Application_Security_FAQ#how-do-i-...
Twitter.com and Google seems to rely on Cache-Control: no-store header on authenticated services.
Fastly article from 2014 states about Vary: Cookie: "Cookie is probably one of the most unique request headers, and is therefore very bad."
Basically just rerolled the patch into MR and added few tests. Tested also manually, so setting the status to RTBC.
+1 for adding this.
I was about to merge the MR, since it would be much easier to test on different projects if the upgrade path would be on -dev, but then I started to think if we should also add the suffix option. When the prefix+suffix was added on #3010955: Allow index name flexibility → it was argued that it makes easier to alter the index for different environments eg.
$config['elasticsearch_connector.cluster.cluster_name']['options']['rewrite']['index']['suffix'] = '_test_env';
not sure if there is an easy alternative now to accomplish the same...
@hexaki managed to make this issue obsolete on 📌 Figure out why test results differ from search_api_opensearch Needs review , so closing this.
Changed few double quotation marks, but changing all of them would make Elasticsearch tests to fail.
The requirement for deprecated configs needs to come from core. Sorry for the hassle, lets keep them.
After handling the prefix I think we should merge this and create issue(s) for 8.x-7.x branch and deprecate the hooks and Events. Scope of the issue would just mark them deprecated and on change record describe how the EventSubscribers should be changed when updating to 8.0.x.
I agree with Phase 2 approach, only small detail is to cover the need to also set the index as read-only during the step two "reindex the old index (foo_green) to the new one (foo_blue)".
I think we should use this issue to cover only phase 1 and use ✨ Support Aliases API and zero downtime mapping updates Active for phase 2.
We should aim that elasticsearch_connector 8.0.x is also compatible with Elasticsearch 7. Noticed that Health API (used on tests at https://git.drupalcode.org/project/elasticsearch_connector/-/blob/8.0.x/...) is not present on Elasticsearch 7-branch https://www.elastic.co/guide/en/elasticsearch/reference/current/health-a...
Since this affects only on tests this does not require any further steps.
Moved 📌 The whole index gets cleared when any change in the search index configuration is imported Active to Nice to have, since the feature is same as in 8.x-7.x. Added related issue ✨ Support Aliases API and zero downtime mapping updates Active to list. We could leave these also for next alpha/beta release.
Lowering the priority since the feature is same as in 8.x-7.x.
Thanks @hexaki! I have looked the MR few times, on the first glance it looked like the changes where totally out-of-scope, but after more reading they make totally sense, excellent work! Its good that we're be able to also cover the parent testBackend! We should include that as release highlights for 8.0.0.
Only minor nitpick about the quotes,
Drupal does not have a hard standard for the use of single quotes vs. double quotes. Where possible, keep consistency within each module, and respect the personal style of other developers.
With that caveat in mind, single quote strings should be used by default.
https://www.drupal.org/docs/develop/standards/php/php-coding-standards#:... → .
I've set the status RTBC, does not hurt if anyone else could also check the changes, I'll re-read the changes once more before committing.
The Search API backend also needs to support the Bigint datatype. The referenced issue for Elasticsearch #3198497: Support 64-bit integer numeric value → has a patch for Elasticsearch and for search_api_db the search_api module needs following patch (for 8.x-1.13 release)
diff --git a/modules/search_api_db/src/Plugin/search_api/backend/Database.php b/modules/search_api_db/src/Plugin/search_api/backend/Database.php
index a7ed3c7f..ee631d42 100644
--- a/modules/search_api_db/src/Plugin/search_api/backend/Database.php
+++ b/modules/search_api_db/src/Plugin/search_api/backend/Database.php
@@ -945,6 +945,7 @@ class Database extends BackendPluginBase implements AutocompleteBackendInterface
case 'uri':
return ['type' => 'varchar', 'length' => 255];
+ case 'bigint':
case 'integer':
case 'duration':
case 'date':
@@ -1683,6 +1684,7 @@ class Database extends BackendPluginBase implements AutocompleteBackendInterface
}
return $value;
+ case 'bigint':
case 'integer':
case 'date':
return (int) $value;
Adding the tests is a bit cumbersome since currently it would require an above patch for search_api module.
Looks good, not sure if we should include tests for this or not.
sokru → made their first commit to this issue’s fork.
Rebased the MR. Looks good and should be committed.
I tried to rebase @camilo.escobar's MR, but apparently created a new one. MR21 was failing because #b1a0d27, so rebasing resolved the issue. Since related issue 🐛 TypeError: Drupal\field_permissions\FieldPermissionsService::fieldGetPermissionType(): Argument #1 ($field) must be of type Drupal\field\FieldStorageConfigInterface Fixed didn't contain tests either and this fixes the issue, I set this RTBC. I also raise the priority since this cause fatal errors on many applications using json:api.
Fixed few remaining CS issues, since the phpcs job succeeds on CI, I set the status to RTBC.
I think this should be closed now.
Thanks!
Confirmed we've same issue as 🐛 The whole index gets cleared/deleted when any change in the search index configuration is imported/synced Needs work . So replaced it on summary with 📌 The whole index gets cleared when any change in the search index configuration is imported Active .
sokru → created an issue.
Thanks! I agree with decision to leave existing lints to phpstan-baseline.neon, we could research it on separate issue and collaborate with search_api_opensearch module maintainers, and maybe doing it before increasing the level further. It seems we get related errors when reaching level 5.
Also thanks for creating ✨ Declare that ConditionGroupInterface or ConditionGroup implements \Stringable Needs review , maybe we should ask feedback from #search or #contribute on Drupal.org's Slack if we don't get any replies in few weeks.
Thanks, looks good! I'd suggest we remove the "temp" and "temporary" terminology from MR. The word "debug" itself should indicate that its not meant to be used on production.
Here's few examples for configuration migration tests:
https://git.drupalcode.org/project/metatag/-/blob/2.0.x/tests/src/Functi...
https://git.drupalcode.org/project/search_api/-/blob/8.x-1.x/modules/sea...
https://git.drupalcode.org/project/feeds/-/blob/8.x-3.x/tests/src/Functi...
8.x-7-0-alpha4 is compatible with D10, if you can provide steps with standard installation profile in Drupal 10 and error you face, someone might be able to help you.
The required package needs to be updated via composer.
eg. composer require nodespark/des-connector:^7.x-dev
If you choose not to use composer, you must manually download the required packages defined on modules composer.json file. I would highly recommend using composer to make the process easier.
Increasing the PHPStan level creates 48 new errors (see https://git.drupalcode.org/issue/elasticsearch_connector-3426825/-/jobs/...), we should fix them in this issue.
One option is to just manually bump the version number every time a new minor is released, but then we rely on our memory/resources to do the change.
I think many Drupal contrib modules use OPT_IN_TEST_PREVIOUS_MINOR in .gitlab-ci.yml to make sure the module has backward compatibility.
This was committed during the 📌 Investigate search_api_opensearch as base for elasticsearch_connector Fixed
Transfered the credits to 📌 Investigate search_api_opensearch as base for elasticsearch_connector Fixed
Created one more follow-up, I hope all the issues from MR are now documented, if not we shall create separate issues from them. I'm merging this now, huge thanks for @mparker17 for this!
Closing this in favor of 📌 Investigate search_api_opensearch as base for elasticsearch_connector Fixed
Adding credits from 📌 Fix failing tests and run tests on DrupalCI Needs review
One more follow-up issue. I'll merge this soon.
Linked the follow-up issues.
sokru → created an issue.
I'm the only active mainer for elasticsearch_connector and I don't have required privileges to add co-maintainers. I'm happy to second mparker17 as co-maintainer, he has been very active on work to get this module to support Elasticsearch v8.
I spent weekend updating merge request on 📌 Investigate search_api_opensearch as base for elasticsearch_connector Fixed . I'd be much rather use the codebase from the search_api_opensearch unless there's a strong arguments to use MR's from this issue. Codebase on both issue introduce significant changes so people who have extended any classes/hooks from 8.x-7.x needs to update their codebase anyway. But I'm open to counter arguments, and if we'd choose to close this in favor of 📌 Investigate search_api_opensearch as base for elasticsearch_connector Fixed I'll transfer the credits to corresponding peoples.
sokru → created an issue.
I've several projects where focal_point is used with Drupal core's media module.
It needs something like this configuration for core.entity_form_display.media.image.default.yml:
langcode: en
status: true
dependencies:
config:
- field.field.media.image.field_media_image
- image.style.scaled_m
- media.type.image
module:
- focal_point
id: media.image.default
targetEntityType: media
bundle: image
mode: default
content:
field_media_image:
type: image_focal_point
weight: 1
region: content
settings:
progress_indicator: throbber
preview_image_style: scaled_m
preview_link: true
offsets: '50,50'
.........
Based on MR 1134, I added two improvements:
1. Fix alignment of grouped radio filters mentioned on #31
2. Move multi-select operator to top of flex element.
Before:
After:
Tests on https://git.drupalcode.org/project/drupal/-/merge_requests/5591 now pass so setting the status back to RTBC.
@larowlan as mentioned on #106, "The MR is a reroll of the test-only patch". The patch still applies and passes the tests.
Regarding the form, the idea is to define the settings at widget level, like in allowed_formats module, not at field level. But this can be discussed if the field level approach is preferred.
+1 for adding it on widget level, so its easier to make allowed_format contrib module obsolete.
Still an issue with 10.1.7. From production logs I noticed the issue was triggered by wrong query parameter on aggregated css/js assets. Oddly it was adding query parameter &theme=site_b, when the only available theme is site_a. We have two drupal platforms, which are hosted by different vendors, but authentication is done with Samlauth/SSO, which indicates the user was first logged into site_b and then arrived to site_a which caused the wrong query parameter to be generated.
For anyone looking a temporary solution with WYSIWYG editor disappearance, I've created a following core patch.
The attached patch should handle the issues found on #100. Also included a patch that shows the failing.
Also tried to trim the summary of the issue to first phrase on the issue summary.
Have been trying to debug the CKEditor field converting into plain textarea after submitting the media field translation dialog.
What I have tried so far:
- Drupal with different core versions (9.5.x, 10.1.6, 10.2.)
- Switch the WYSIWYG field to use CKEditor 4
- Hardcoding the core/ckeditor5 dependency to module and #attached libraries.
Once this is resolved I think it would greatly help to tackle the regression if we could add the tests. Wrote the manual steps how to reproduce the issue.
composer require drupal/admin_toolbar drupal/media_library_edit:3.0.x-dev drush/drush
vendor/bin/drush en content_translation media_library_edit media_library -y
cd modules/contrib/media_library_edit
curl -O https://www.drupal.org/files/issues/2023-09-11/3316163-22.patch
patch -p1 < 3316163-22.patch
# Add a new language
http://localhost:8888/admin/config/regional/language/add
# Allow translating node:article
http://localhost:8888/admin/structure/types/manage/article
# Allow translating media:image
http://localhost:8888/admin/structure/media/manage/image
# Create a new media reference field with media:image
# Make sure "Users may translate this field" is checked
http://localhost:8888/admin/structure/types/manage/article/fields/add-field
# Make sure "Show edit button" is checked on Media field
http://localhost:8888/admin/structure/types/manage/article/form-display
# Add a new article and add media:image
http://localhost:8888/node/add/article
# Translate the article and edit the media field.
# After submitting the media dialog, the CKEditor field changes to plain textfield.
Apparently this is not so rare usecase. +1 for RTBC. Included a related issue that should be closed if this gets fixed.