🇳🇿New Zealand @ericgsmith

Thanks @megachriz

Changes and comments all make sense to me - looks good!

Thanks @dydave - appreciate the quick update, that should hopefully save a few people updating before 3.5.3 is tagged. Cheers

Reverting I think is good - thanks @tonytheferg and @dydave.

It would be awesome if this can please get committed and release a 3.5.3 version.

The release notes for 3.5.2 also do not mention issue ✨ Add re-index menu Active - they should be updated to include that and also list this issue as a known issue.

Moved existing patch to MR.

I note that while this has been RTBC for almost 2 years there is mention in 📌 Refactor module architecture in a simpler, opinionated and more performant approach Needs review of removing views integration completely.

I have a use case that I am exploring using views to solve - and this patch gets me somewhat closer. Given this does a join from entity_usage to the entity its ignoring the vid - it would be good if we could either chose to join to the revision table instead of the base table, OR allow the relationship to have an additional config to add 'field' => $entity_type->getKey('revision'), 'left_field' => 'source_vid' to the

relationship.extra

definition in order to only relate based on the default revision.

I'm more curious about the future of the views integration for this module - are further patches welcome or is the future for views integration a different module?

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

pfrilling → credited ericgsmith → .

greggles → credited ericgsmith → .

Went back to have a look at the project we were using for this.

Originally when using Solr 8.x we had enableRemoteStreaming set to true through some custom request dispatcher config - something like:

search_api_solr.solr_request_dispatcher.request_dispatcher_remote_streaming.yml:

uuid: ....
langcode: en
status: true
id: request_dispatcher_remote_streaming
label: 'Remote Steaming'
minimum_solr_version: 7.0.0
environments: {  }
recommended: true
request_dispatcher:
  name: requestParsers
  enableRemoteStreaming: true
  multipartUploadLimitInKB: -1
  formdataUploadLimitInKB: -1
  addHttpRequestToContext: true

In later solr version this changed to being enabled by an environment var - so now we just have an environment variable:

SOLR_OPTS: "-Dsolr.enableRemoteStreaming=true"

But before fill you with false hope - we were using S3FS module but with the public file takeover, meaning the bucket is publicly accessible and the external URL is used. I haven't tested with the non public wrapper.

Tested this and it works perfectly. Old key value entries were cleared once I configured to use the file cache, and the cached extracted files persist through cache clears when preserve_cache is true and are correctly deleted when preserve_cache is false. Both methods continue to use the cache when available when indexing.

As far as I can see the open discussions on the MR from Frank have all been addressed in the last commit and I agree with the proposal to only select the scheme rather than a specific directory for the storage.

CI showed a minor typo and deprecation warning - once those are applied I think this is RTBC as far as I am concerned.

Thank you everyone who worked on this - file storage is generally cheaper than database storage so there a potential this change will not only be more performance but could save a few cents here and there. Looking forward to seeing this land soon 🙏

We've been using this module with s3fs for a long time with no additional patches or code changes needed but from memory solr needs to be configured to allow remote streaming

Made some changes.

I found that while on a local site I had target_entity_type_id in config, when I tried testing on a fresh D11 install it wasn't there - it was only discoverable by loading the argument handler (which supplements the item with the views data - which specifies the target entity).

I then found that the term filters don't actually extend this anyway - so I've added a slightly non elegant check for the term filters.

I see there is currently not CI setup for the project - I can raise an issue for this as it would be good to get that setup before testing / reviewing this.

I'm picking this back up as a client wants to move forward with this functionality.

Having looked at Added EntityArgument and EntityReferenceArgument Views argument plugins → it seems for my use case the code can be simplified a lot if I only care about 10.3.x and above as I can now find the target_entity_type_id property in the argument definition instead of parsing the fields like previously.

I have only given this a quick test with some views that have taxonomy term id contextual filters and it works as expected.

@a.dmitriiev I have not set this to needs review yet as I plan to tinker with it a bit more in the next few days.

Thanks for reviewing and sorry for the delay!

ericgsmith → changed the visibility of the branch 3504114-cache-redirect-error to active.

ericgsmith → changed the visibility of the branch 3504114-cache-redirect-error to hidden.

ericgsmith → created an issue.

Thanks @megachriz and @ptmkenny for the continued efforts to push this forward. I think this is a really good fix and will help reduce a bit of confusion and extra work currently needed when using Tamper.

I've reviewed the recent commits to the MR and everything looks good to me - the addition of the test methods to the base class is great and will help us ensure we are thinking about this problem when developing new plugins.

I've manually tested a few of the plugins on an existing site without any issue - and I was able to remove a lot usage of 'SkipOnEmpty' that we had been using as a work around and its all working as expecting. Great stuff! I did not test them all manually but the automated tests give me confidence in them all.

In case of an empty string, the number of words should be zero. The WordCount actually returned 1 in this case, so additional code is added to fix that.

This sounds like a good catch and worth highlighting in the release notes since it won't be obvious in this issue title 👍

I'm setting as RTBC - I'd be happy to merge now @megachriz - I know you have a lot of open issues at the moment so will let you decide which ones to merge first to minimise any rebaseing effort needed. Given the additions to the base class this one might be a good candidate to go in first.

Great work everybody!

ericgsmith → created an issue.

Thanks @mably - I've rebased the MR and updated target to 2.1.x

bojanz → credited ericgsmith → .

Better late than never! Not sure why I didn't merge this at the time - have been using the patch for over a year.

Test passed locally:

docker compose exec php-cli phpunit -c /data/app/core/phpunit.xml /data/app/modules/contrib/address/tests/src/Kernel/AddressTokenTest.php
PHPUnit 10.5.39 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.14
Configuration: /data/app/core/phpunit.xml

...                                                                 3 / 3 (100%)

Time: 00:16.928, Memory: 12.00 MB

OK, but there were issues!
Tests: 3, Assertions: 58, PHPUnit Deprecations: 1.

Needs 📌 [Gitlab] PHPUnit tests fail after --verbose removed, and no scheduled pipelines exist Active to pass in CI as there is a bit of work to get the pipeline green - good enough for RTBC for me :)

Made a start on getting everything green again

CSpell - Most words looked project specific, the only ones that I wasn't sure on are there are a few UI labels using British spelling (e.g. Neighbourhood instead of Neighborhood) - it looks from a quick scan that this was withing the context of screens where the US spelling is presented (e.g Organization instead of Organisation) so I assume these are genuine misspellings. Not fixed given they impact UI - wanted clarification first.

CSpell - organisation looks like it was used in the migration plugin - so its in the dictionary but perhaps should be just ignored in the plugin?

PHPCS - minor fixes

PHPStan - updated the baseline to ignore some deprecations related to things introduced in 10.3. Since 10.2 went out of security support this week, maybe we should just fix these now and have min 10.3 support? Will take maitainers direction here.

PHUnit - version 10 needs dataprovider related methods to be static - made these changes.

There are still 19 failures - 18 will be resolved by 📌 Drupal 11 compatibility fix for removed views default_argument_skip_url setting Active

The final one is:

18) Drupal\Tests\address\Unit\Plugin\Validation\Constraint\CountryConstraintValidatorTest::testValidate with data set #3 ('InvalidValue', 'The country %value is not valid.')
Prophecy\Exception\Prophecy\MethodProphecyException: The method "addViolation" has a void return type, and so cannot return anything
/builds/issue/address-3495167/vendor/phpspec/prophecy/src/Prophecy/Prophecy/MethodProphecy.php:256
/builds/issue/address-3495167/tests/src/Unit/Plugin/Validation/Constraint/CountryConstraintValidatorTest.php:60
/builds/issue/address-3495167/vendor/bin/phpunit:122

I have to divert my attention to something else now but its a start.

Setting to needs work for the above - but I think adding a scheduled pipeline is not blocked as if that is done now it will make it more visible that this is the current state. Cheers

ericgsmith → created an issue.

Thanks all - we've been using this patch for a couple of months and it works perfectly for us.

Code changes look good and so does the test 👍

Moving patch from #9 to a MR against 2.1.x

Looks like tests and a few jobs are failing but they may not be related to this change as it looks like the pipelines aren't running on a schedule and I can't see the last run - I'd RTBC it but will investigate these failures first.

ericgsmith → changed the visibility of the branch 3350959-add-token-for to hidden.

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

Moved the existing patch to an MR.

Tested again and Len's comment is #42 is correct - this patch causes an error when following the steps to reproduce. Once you add the new view and before saving it the preview will through an exception as we are trying to get a route for a view that does not exist.

Once the view is saved the validation recommendation is resolved - leaving as "Needs work" as the approach to resolve the issue does not have consensus.

Have tested on 11.x and it still producing the same recommendation. Updated the issue summary steps to remove reference to drupal 7 and use terminology in the current views UI - also tweaked to title to clarify this is a recommendation not an error.

It seems that this problem is specific to the class attribute

This is the most common example but this problem is more generic and there have been multiple comments with examples of this. I feel like special handling for class attribute is going to help maybe 95% of the people in this issue, but there is not reason why we can't help everybody.

I have previously voiced support for reverting in #47 and still hold that opinion.

ericgsmith → created an issue. See original summary → .

Please see the project page description - this project is for historic purposes and should not be used for new projects.

Please see the project page description - this project is for historic purposes and should not be used for new projects. The original steps to use the project in the readme are still accurate which will prevent this error if followed - but as above - do not use this project for a new theme.

Please see the project page description - this project is for historic purposes and should not be used for new projects.

Found this issue while looking 🐛 Referencing new media requires "administer media" permission Needs review which is dealing with the same issue - only for the view own unpublished media permission / MediaSelection handler. I set that issue to postponed as I believe the backwards compatibility handling and UX review happening here are needed and applying the solution from this to the media selection could be done as a follow up to this issue.

Looking at the feedback from the accessibility review

> At the moment you are unable to distinguish between published and unpublished nodes while scrolling the list of available results. Unpublished nodes should be labeled in the list of options in the autocomplete field in some way.

I would suggest it should be a follow up, since this change is not introducing this problem. Seeing unpublished content is currently the case for users who see unpublished content by way of the existing checks so changing the label could be done independent of this work?

I have rebased the MR against changes latest - looks good to me.

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

I have expanded some tests - from a quick look I couldn't find existing tests covering the new entities behaviour - the validation constraint looked to be the appropriate place to add that.

Once 🐛 User can't reference unpublished content even when they have access to it Needs work lands with the include_unpublished_entities setting on the base handler that can be incorporated here. Setting to postponed based on that.

I have started expanding test coverage for the selection handler - end of the day for me so assigning to me, still need find where the validate new references is tested.

Added additional permission check mentioned in #28 - have not added this to the validate method yet - that is still to be done.

I still believe this will need to wait for 🐛 User can't reference unpublished content even when they have access to it Needs work to land as that introduces a BC layer for this logic change to the base handler - I think it makes sense to wait for that issue to land rather than trying to work that in now and likely this could be postponed as a follow up - but I will try complete the remaining work soon as I need this functionality now and do not have a concern about it being a BC break.

Made a few minor fixes for CSpell and PHPStan jobs.

@albert volkman did you get a chance to test this, or only looking at upgrade status for now?

I'm keen to get this merged in - I'll leave it for a couple days and merge if I don't hear anything else.

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

We probably also do need an empty hook_post_update_NAME to force a cache clear

Rebased MR + opened a new MR into 2.0.x branch.

For me this is a bug fix - so can go into either - but depending on your approach may need BC handling. I'll note that Drupal cores BC handling for event subscribers says:

Class implementations of paramconverters, access checkers, event subscribers and similar services which are never expected to be used directly either as services or value objects, are not considered part of the API. You should not extend from these classes and provide your own implementation instead. (However, note that service machine names are public API and changing them requires a deprecation process.)

So I guess technically you could keep the service definition in place with an empty subscriber. Not sure what that would achieve really - also 2.0.0 is released so if the argument here is that its a BC break then you will need 3.x - I think that would be overkill for an internal class.

Rebased against 2.0.x - minor changes made to constructor to get the commit to apply cleanly.

Hiding old MR and branch against 8.x-1.x

I note there is not 2.x or 2.1.x branch - as this is a new feature it would need to go into a 2.1.x release at this point no branch existis for this.

Updating IS and changing to needs review now that change in xls_serialization landed

Moved patch #7 to MR

This was previously RTBC'd in #9 and #10 and #11 but subsequent patches were added which moved it back to needs work / review.

I'm incline to argue the subsequent changes to the patch have not been needed and the tests were lost - I'm leaving this as needs review.

Rebased #25 patch did not apply cleanly after changes in 8.x-1.x (use statement order changes only) and opened MR.

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

My opinion is that there is no need for a 2.x branch for this change nor the change to the protected method as the BC policy is pretty clear about service constructors and protected methods.

That said - you have already started a 2.x branch so the work is largely done.

Lets move this convo to 📌 Rollback BC breaking change for encoder constructor Active since the constructor change was nothing to do with this issue

Thanks @stefanvaduva for the review and picking that UI bug up - I have pushed a fix for this.

@max-ar thanks for the comment. Interesting that the missing json field ends up as an empty array - that warrants further investigation and would be helpful when evaluating ✨ Add has() method to ItemInterface Active

Re the typo in the protected method there is a policy for bc changes which mentions that protected methods are internal and subject to change https://www.drupal.org/about/core/policies/core-change-policies/bc-polic... →

I think it's totally fair to fix a typo in that method without needing a major version bump.

🐛 CKEditorError: plugincollection-plugin-not-found Needs work eventually did the same fix as MR11 - I had missed that this was reopened as active rather than needs review - happy the fix got in eventually :)

🙌 🙌 🙌

Quick test with same project I mentioned in #12 - using twig 3.14.2 and the patch from MR10177 at commit 67bbb312 which applied cleanly to 10.3.6

After drush cr

"duration":4.683,"cpu_user":47.20,"cpu_system":5.98,"cpu_total":53.18,"memory_peak":137277440

(original 3.14.0 duration was 4.943 so back to normal)

Subsequent load with a warmed cache:

"duration":1.559,"cpu_user":86.62,"cpu_system":7.70,"cpu_total":94.32,"memory_peak":107917312

(original 3.14.0 duration was 1.550 so back to normal)

Only did the comparison a couple of times with slight variance in times but certainly appears to resolve the performance issue with no noticeable side effects.

Have not done a thorough code review but good to me - thanks Lee!

Doesn't rebase cleanly against latest changes in 8.x-1.x branch - will take a look soon

All green after rebase - previous code style issues were not related to this change and fixed in 📌 Drupal 11 compatability Active

Assigned to myself to rebase and get pipeline green

I checked the logs locally - loading the permissions page on a project with several contribute modules and 8 roles (and with env var XDBEUG_MODE: off) does show a big difference in response time.

First run on twig 3.14.0

After a drush cr:

"duration":4.943, "cpu_user":42.08,"cpu_system":5.26,"cpu_total":47.34,"memory_peak":135180288

Subsequent load with a warmed cache:

"duration":1.550,"cpu_user":86.44,"cpu_system":5.81,"cpu_total":92.25,"memory_peak":107917312

After updating to twig 3.14.2

After a drush cr:

"duration":14.603,"cpu_user":83.95,"cpu_system":1.78,"cpu_total":85.73,"memory_peak":135180288

Subsequent load with a warmed cache:

"duration":11.857,"cpu_user":97.83,"cpu_system":1.18,"cpu_total":99.01,"memory_peak":107917312

I have not noticed the degradation other than locally. When xdebug is enabled it's unusable in places, pages like the permission page fail to load (timeout). On environments without xdebug I didn't notice a difference but have not profiled or measured it.

@mably thank you for paying attention to this module 🙌 We use this on a handful of sites and its good to see some tidy up. Changes look good to me, just a minor comment added to MR.

I think that the fact view any unpublished content applies to any entity is confusing - and adding help text is not going to help whenever I see this used in code or site config.

I think the fact that content moderation module is providing a permission that applies to entity types that are not moderated is also confusing. That said, I find it useful it does this even if view any unpublished entity would be a better name.

So big +1 to solving this by renaming or refactoring the permission.

I think similar to the related issue - this check should also take the permission view any unpublished content into account since view any unpublished content also applies to media 📌 'View any unpublished content' permission is surprisingly broad Active

Perhaps this is realistically postponed until 🐛 User can't reference unpublished content even when they have access to it Needs work lands so that the BC impact can be the same, once that config in the related issue is added it can be used here>

Related issue 🐛 User can't reference unpublished content even when they have access to it Needs work is talking along similar lines but for content, not media. To me this feels like a shared issue around unpublished entities being hidden from users who have access to them.

ericgsmith → created an issue.

Opened MR for that - https://git.drupalcode.org/project/views_aggregator/-/merge_requests/29/...

I see now that this is not really to do with the original summary only the last few comments in this issue - let me know if you want me to open as a new issue instead.

Hi @tr

I have also experience this same result after updating.

It looks like there is a missing . in views_aggregator_update_10200 between the key and property for when the current values of precision and totals_per_page are fetched which is then not getting the actual values and setting them as 0.

E.g. the value comes from $key . 'column_aggregation.precision' and is set to $key . '.column_aggregation.precision'

ericgsmith → created an issue.

That deprecation of Drush\Drupal\Commands\sql\SanitizePluginInterface is not a blocker / change needed for Drupal 11.

That new class was introduced in Drush 13 - and the deprecated class still exists in 13. As 13 requires PHP 8.3 I think its reasonable to continue to support Drush 12.

Thanks @rosk0 - removed the duplicate comment - thank you for the reviews.

Patch correctly identifies and fixes the issue - just a minor suggestion to the fix on the MR but really will leave that to maintainers as its more a code style suggestion to keep the number of returns in the function the same.

Rebased MR after conflicts introduced when 🐛 No longer a way to post a link as-is if there is 1 pattern match Active was comitted.

Completely missed the first time around - but there are existing in the schema for xls settings - they are just not exposed in the UI anywhere.

It would make sense to add these to the form as well - I am only specifically interested in this once the related issue lands in in xls serialization - so I do not plan to commit more time to this right at this moment.

Tests pass, gave it a manual test and works as expected on D11 - fixed the phpcs and cspell jobs.

Would be awesome to get this an as I am working on several other issues that would benefit from having the tests running.

ericgsmith → created an issue.

MR is now testing on Drupal 11 by default - which this module does not have a release for - so ran manual pipeline against Drupal 10 - https://git.drupalcode.org/issue/xls_serialization-3276294/-/pipelines/3...

Setting to needs review.

Please note the pipeline is failing as the gitlab ci config is to test the current major - which is Drupal 11 - but this module is not Drupal 11 compatabile.

I run the pipeline with the variable set to test on the previous major (i.e test on Drupal 10) and we can see the test passing here https://git.drupalcode.org/issue/xls_serialization-3362321/-/pipelines/3...

This issue is not only applicable to the rest export - it is also applicable to the excel export and views data export modules when authentication is in place.

I have fixed this with 2 changes

I have made changes so the encoder more flexible for the data it is provided with. At the start of encode the encoder is casting everything to an array, but from that point it assumes that it has a nested array. It also makes an assumption that the row keys are numeric - but they do not have to be. So changes have been make to fix the type errors.

Once the issues in encode were fixed the endpoint was still returning errors:

Exception: Deprecated function: stripos(): Passing null to parameter #1 ($haystack) of type string is deprecated
Drupal\Core\EventSubscriber\ActiveLinkResponseFilter->onResponse()() (Line: 85)

This is due to this module using an event subscriber to add the format. This was one common, but now the recommend approach is to use a service provider to register the formats as middleware. CSV serialization implemented the same change 🐛 PHP 8.1 deprecated function warning Fixed which is based on how several other serializers are doing this, e.g:

- https://git.drupalcode.org/project/hal/-/blob/2.x/src/HalServiceProvider...
- https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/jsona...
- https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/file/...
- https://git.drupalcode.org/project/csv_serialization/-/blob/4.x/src/CsvS...

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

Duplicate of 🐛 The serialization fails if used with Views REST export Active - I will move the test over there

Added test to show the error

ericgsmith → changed the visibility of the branch 3481346-php-errors-when to hidden.

ericgsmith → created an issue.