Thanks @catch - tidy up looks good - I've added an additional comment to cover why we are repeating the publish / unpublish check.
Hello 👋
I can't offer any insights to the original design - I became a maintainer in 2023 in order to get a D10 release out - and as part of that process I never heard back from @nielsvm so I am unsure if they would see this.
My focus as maintainer since this has been trying to progress issues that are either RTBC or relevant to the projects I work on. So as such if there is benefit to improve here and somebody is willing to do the work (including tests to prove it works and that its beneficial) I would support it and be available to review / commit etc.
Thank you @catch and @kristiaanvandeneynde for all the action so far here and on 🐛 Cache redirect error displayed when using 'view own unpublished content' permission alongside node grants Active . I have been meaning to come back and provide feedback (and thank you for all the explanations!) but needed to find enough time to digest it all.
Looking just at this issue - this change resolves the error I was seeing and seems like a good quick win.
Reviewing the previous comments - I wasn't sure if this comment 🐛 Cache redirect error displayed when using 'view own unpublished content' permission alongside node grants Active from @kristiaanvandeneynde was implying there would still be issue with this approach? I got a little lost tracking which comments were to which MR / approach.
The test I originally wrote as a quick and clunky way to just trigger the error - at a minimum some of the comments need updating - but I wonder if we need to actually assert for something to make it more useful? Or alternatively simplify it to check user.node_grants:view is consistent in the cache metadata returned from node access. I'm happy to take suggestions here and tidy up / work on the tests.
I don't know how this module integrates with content moderation (that would be outside the scope of this issue if anything is not working with it) but I've fixed the annotation which had the wrong command name in it
Looks like there is a mistake in the annotations, the example usage and actual command name do not match, I'll fix work on that
I couldn't update the MR to go into 3.x branch - so I've pushed a new branch 3024419-fix-required-fields-3.x
If somebody is able to update the MR I'll push my rebase there. I also added test coverage for the use case I hit which is with a multivalue entity reference field.
I'm not sure if we want this as its own test, but since its just being added to the entity and not actually interacted with which is the issue I just expanded the current test. Happy to take maintainers direction here.
ericgsmith → made their first commit to this issue’s fork.
If we use kernel.terminate then the code will not run if page cache is handling the response - in the current state this is good as the module tries to skip this scenario anyway, but this has a potential side effect if https://www.drupal.org/project/purge_queuer_url/issues/3045503#comment-1... ✨ Registry expiration as opposed to removing it too soon Needs review means we need to be able to know when the page is served by page cache.
Will need a rebase after 🐛 Logged-in requests remove items from traffic registry RTBC comitted.
While that related issue is committed I am still keen on this idea as a solution.
Closed #2881045: Add/remove sessionless requests only → as a duplicate, assigning credit here.
Closing as a duplicate of 🐛 Logged-in requests remove items from traffic registry RTBC
I note that the solution here is different in that it adds configuration for the change - I see this as a bug and not something that needs to be opted into.
I will transfer credit over to that issue.
Thanks all involved!
catch → credited ericgsmith → .
Added test coverage, would be good to cover more scenarios but out of scope for this issue. Will tidy up the cs issue in the morning.
ericgsmith → made their first commit to this issue’s fork.
Added a check to cover the remaining check that the field was requested mentioned in #9
I haven't moved this to Github yet - I think getting buy in for the approach first since this has been open for a while.
While I think conceptually "Index empty fields" is probably a more logical thing to opt in to, this looked to have a few moving parts I didn't understanding while debugging.
ericgsmith → changed the visibility of the branch 3179643-empty-field-leads to hidden.
ericgsmith → changed the visibility of the branch 8.x-1.x to hidden.
ericgsmith → changed the visibility of the branch 3179643-empty-field-entity-load to hidden.
I'm having a different issue but it feels very related to this as the issue stems from the behaviour for empty fields.
In my case the field is a property added by a processor - I'm not sure if that fits in the definition here of a field.
In my case, I have "Retrieve result data from Solr" set to true and I am using the search api attachments module to index document contents. I am using that field in views.
When the field is empty I can see the same outcome as the issue summary when the field is pre rendered - !isset($row->$dependent)) is TRUE so the field becomes required. Rather than loading the entity, this now diverges from the issue summary and loads the processor as calls addFieldValues.
I have tried using the "Index empty full fields" option but when it comes to indexing it gets to the loop where values are added - but at this point $values is an empty array so nothing is added to the index.
Applying this patch seems to resolve the issue - the empty field is added to the result set and then when the check is done in the getValuesToExtract the value set is an empty array and no action is taken.
I will do a more thorough test & review in the next few days.
Fair amount of PHPCS issues that need manual fixes.
Setting to needs review as IMO having this in place to allow testing is better than nothing, even if its reporting PHPCS failures. Can always fix them in another issue since the job is allowed to fail.
ericgsmith → created an issue.
Moved patch to gitlab to hopefully see this get committed - we have been using this patch for coming up 20 months without issue.
Tests are not running in gitlab but still pass locally, I will open another issue to get CI setup.
Alright, gave this another crack and for whatever reason I need to repeat the unpublish dance to generate the error, and now have the error generating in CI - https://git.drupalcode.org/issue/drupal-3504114/-/jobs/4579941#L1064
Is this a cache context optimization issue?
I've reverted the change made in e49931def619c244d72fe6f12eb9bacd5311bf14 that was subsequently bugged in the rebase and causing a test failur - that test change is unrelated to this MR and the existing test from the 2.0.x branch should still pass as there is no expected behaviour change here.
There is another unrelated test change there that should be reverted - missed it before I pushed my change.
ericgsmith → changed the visibility of the branch 2.0.x to hidden.
ericgsmith → changed the visibility of the branch 8.x-1.x to hidden.
Adding 🐛 Render using theme input and select instead of lists with links for checkboxes and dropdown Needs review as a related issue, as that issue also seeks to remove the rendering of links from the checkbox widget although in a different approach
I have pushed an update to the MR with a change that allows specifying if the join should include the condition to match the source vid as well.
I appreciate this might over complicate it.
The original commit on the MR (552e2ea7452c5a3e42b78b952e573e1c8e55fe42) is what was RTBC'd in the past.
If this addition plugin just to add the join is over the top, perhaps we could either always add the condition or add 2 different relationships? I always get confused around the labels for default / current or however they are called.
I didn't want to highjack an issue that was previously RTBC, but since this has had so much timeout without being committed I think its worth disucssion this join criteria a bit more - and as above it would be good to know the future of views integration for this module.
Thank you for moving the issue. Unfortunately I don't have the knowledge to say this is a node issue and not a cache issue?
I have had trouble really digging into the variation cache as I think I can't find documentation on some of the basic terms like cache redirect - and I'm confused why the redirects which appear to relate to this warning don't contain cache tags. There is no doubt a good reason, but I don't know enough about that to say that this is a node issue.
It seems like the cache has a dependency on a node - and that node changes so the cache is correctly invalidated, but whatever these redirects are seem to hang around due to the absence of tags. Now the node has changed that brings about a scenario where the context is then different - and that triggers an error.
The test WIP I added was just to highlight the difference, not to say that the difference is unexpected.
Also - as far as I understood the cache context docs, user.node_grants cache context should specify that the item is uncachable - so I'm futher confused how we end up getting cache redirects errors for something that should not be cached.
Yes - I noticed this while investigating the general performance of search pages on a site with a very large menu.
A few factors on the site lead it to be quite noticeable - which other sites may be susceptible to. Firstly since menu block hides the system block when adding blocks, we've ended up using menu block for all menus - which includes the very large main menu block. In addition to this, there were additional menu blocks for the footer menu, header menu and sidebar navigation block (which is also large as the search page in question is within a sub section of the site). All up that means a lot access checks for the menu. (It turns out, we aren't event using any of the features of menu block for the main menu - so editing the config to us the system block is also a solution for us). I guess because menu block hides the system block, many other sites may be using menu block for their primary navigation block.
Using php-spx it showed that it was using around 200 -> 250ms in the menu block block access method - so for us not a micro optimisation, this is a big chunk of the request. Again that is in the context of 5 menu blocks and a large menu - not all sites will have this issue.
Both the patch and the approach of changing to config to use system menu blocks improved page speeds by around .3s where the page is unached but the block cache is used (and no noticeable change in response time for when the block is uncached). There is a benefit of lower memory use as well given the in memory build cache is removed.
I don't think that time will be applicable for all sites, and I haven't profiled on a clean install - but for me it shows enough that its important to fix - and given the performance issue was already previously raised it will be beneficial for all.
My concern really is only that the people reported the previous patch in the related issue didn't work for them.
If we absolutely need to keep the option of using block access for this, I think the 'display_empty' behaviour should be changed so we have to explicitly opt in to the access check.
I'll note that returning empty did not work previously for some people - https://www.drupal.org/project/menu_block/issues/3271218#comment-14499696 🐛 Menu block renders when tree is empty as of 8.x-1.8 [regression] RTBC
But I don't know the steps to reproduce that since it worked for others, if we can get conclusive steps to reproduce I have time to write a test for it.
I believe this caused a performance regression - 🐛 Performance issue caused by rendering in access check Active
If anybody who experienced an issue that was fixed by this issue are able test that 🐛 Performance issue caused by rendering in access check Active does not cause a regression, or outline some steps that I can add to the automated tests I would be very grateful.
Setting MR to ready.
I've captured where I believe the display empty bug is - in that if the tree is empty a render array is being returned. Now if display_empty is false it just returns empty here.
There are a couple of existing empty array return points in build that I believe are not compatible with the display empty option - this likely needs further testing but that is not the point of this issue, this is to remove the rendering in the access check.
The test passes on the MR and the test only changes pass - so I believe I have not reverted the empty behaviour intent here - but it probably needs more testing from people who previously needed such behaviour.
I see there was a feature added with some config for display_empty which makes this harder to revert.
We can at least change the order here so build is only called if needed - but that will still leave the issue there for anybody with display_empty FALSE.
Honestly I'm confused to how the block is visible when empty - I need some steps to reproduce because when there is no active child and the block returns an empty array nothing is rendered for me. Either way, if this logic is needed it should be moved out of an access check and into the build method.
ericgsmith → created an issue.
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:122I 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
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.