Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Note: a fix for this issue has already been committed to dev, this issue is only open for adding test coverage.
Problem/Motivation
The module has not been updated for https://www.drupal.org/node/1959668. The result is that published nodes can become 403s after a node access rebuild.
This is a critical issue for this module because without it Drupal 8's core multilingual capability completely breaks it.
Proposed resolution
Fix the grants to account for multilingual sites and add tests.
Remaining tasks
Commit a fix- Review/commit automated tests
User interface changes
None
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#33 | 2784887-33.patch | 209.07 KB | achap |
#32 | 2784887-32.patch | 8.91 KB | achap |
#30 | 2784887.30.patch | 208.88 KB | achap |
#6 | 3-6-interdiff.txt | 610.6 KB | alexpott |
#3 | 2784887.3.patch | 9.67 KB | alexpott |
Comments
Comment #2
alexpottComment #3
alexpottComment #4
alexpottThis really should be tested too. I've manually tested it and it works great but a node access rebuild in a hook_update_N() deserves a test.
Note as this module has no tests automated testing is not enabled - so we have a checkin and egg situation. Bascially on d.o. in order to have a green automated test you need to have tests and then you get issue testing.
Comment #5
alexpottDiscovered #2785155: _node_access_rebuild_batch_operation uses queries that check access whilst working on a test for the update.
Comment #6
alexpottSo rebuilding during hook_update_N is not advisable ATM so let's just set the flag and be done.
The patch is huge because it has a database dump for update path testing.
Comment #7
alexpottNoticed the test name and file name were not aligned.
Comment #8
amateescu CreditAttribution: amateescu for Chapter Three commentedLooks like the latest patch doesn't include the database dump anymore..
We can use
Url::fromRoute()->toString()
here :)Comment #9
alexpottThanks @amateescu
Comment #10
amateescu CreditAttribution: amateescu for Chapter Three commentedLooks ready to me :)
Comment #11
drupalninja99 CreditAttribution: drupalninja99 at Mediacurrent commentedI applied the latest patch, ran updates, cleared caches, rebuilt permissions multiple times and still has some pages not showing up. I ended up having to back out the module for now as it's too unreliable.
Comment #12
JeroenTTried this module and it worked for me.
Comment #13
flyke CreditAttribution: flyke commentedWhen I use this module, items go missing in views.
Setup / steps to recreate:
- multilingual site, English as main language, Dutch as second language
- at this point view_unpublished module not installed
- create a node in English, unpublished
- translate the node in dutch and publish it (only the dutch version)
- create a view that shows the nodes of the node type that are published and in the current language
Results (without view unpublished module | visit as anonymous user):
- Site in English: view does not show the unpublished english node (as expected)
- Site in Dutch: view shows the published Dutch node (as expected)
Adding the module (with patch)
- drush dl view_unpublished-8.x-1.x-dev
- download the patch 2784887.6.patch and apply it.
- drush en view_unpublished -y
- rebuild node access via /admin/reports/status/rebuild
- also (to be sure) via drush: drush php-eval 'node_access_rebuild($batch_mode = TRUE) '
- visit the view again on the website
Results (with view_unpublished module | visit as anonymous user):
- Site in English: view does not show the unpublished english node (as expected)
- Site in Dutch: view does NOT show the published Dutch node (NOT as expected)
(p.s. visiting the site when logged in does show the nodes)
Disabling the module and the node is visible again as it should.
After these testings, I also applied the patch from https://www.drupal.org/node/2786109
Does not make a difference. Nodes still not visible.
For me the status 'reviewed and tested by the community' does not seem correct. Looking at the comments here it seems that this module works for 2 people and does not work for 2 other people.
I did notice however that after editing a node that is missing but should be there, and save it without modifying anything on the node, the node finally appears like it should. But I am not going to resave hundreds of nodes just to get this working.
Comment #14
ayalon CreditAttribution: ayalon commentedI have the same issue. This module breaks views listing for multilingual sites. Some nodes are missing for anonymous users and I did not find the difference between the shown nodes and the hidden nodes.
Comment #15
ayalon CreditAttribution: ayalon commentedI think I know the reason for the missing nodes in the views:
It occurs, if you have a view and add a relationship to from type "Content: field_wahtefer_ref".
If the field on the node is empty, then the node if filtered out:
First access filter
Second access filter
In the second access filter a check is done on the relationship and this leads in filtering out the node.
It's not related to multilingual I guess. I will create a new issue.
See here:
https://www.drupal.org/node/2846948
Comment #17
amaria CreditAttribution: amaria commentedThis has been committed to dev. Thanks for the great work @alexpott! I tested everything and ran the tests you included. I made a few tweaks to some of the code to replace deprecated functions with their current counterparts, otherwise code is committed as-is.
@drupalninja99, and @flyke please review the issue setup by @ayalon to see if it applies to you. If not, please open a separate issue detailing the problem.
Comment #18
alexpott@amaria you can now enable automated testing on the project because there's a test!
Comment #19
amaria CreditAttribution: amaria commentedOh right. Thanks. Actually, there was a test already I just hadn't enabled the automated testing.
Comment #21
amaria CreditAttribution: amaria commentedI removed the tests as they were failing Drupal's automated testing, and I'm not exactly sure why. Running it using their test command (without the sql stuff) passes locally. If you can figure out the issue, feel free to upload a new patch containing just the tests and fixtures, to see if it passes and I'll re-commit them.
Comment #22
hoebekewim CreditAttribution: hoebekewim as a volunteer commentedRe-roll to match the current dev version.
Sorry for the missing interdiff.
Comment #23
achapThe patch in #22 failed testing with this message:
Is it possible that the patch you created didn't remove the previous declaration?
Comment #24
achapUpdated test to use publish/unpublish checkbox that was introduced in Drupal 8.4 in favour of the 'Save and unpublish' and 'Save and publish' buttons, as well as declaring test dependencies in info.yml to see if that fixes the Drupal.org tests.
Comment #25
achapAttempt to fix this error message "error: cannot apply binary patch to 'tests/fixtures/view_unpublished_pre_8001.php.gz' without full index line"
Comment #26
achapOne more try, with fixture provided by core. Tests still passing locally.
Comment #27
achapUsed standard install profile, fixed spelling mistake.
Comment #28
achapComment #29
achapComment #30
achapComment #31
achapApologies for the numerous tests. My patch in #30 passes all the tests locally, including SQL update tests when run through the testing module but for some reason it fails in Drupal CI. It fails a Drupal 8.4 test on CI with this:
Not a very helpful message.
Then it has another error (presumably because the first failure meant no nodes were created):
Does anyone know why it's failing on the the assertLinkByHref() ?
Comment #32
achapDrupal CI runs in a subdirectory, so I set up my local environment in the same way and was able to replicate the failures. Hopefully this one works.
Comment #33
achapComment #34
achapComment #37
achapComment #38
kruser CreditAttribution: kruser commentedYep, this is still broke.
Comment #39
rgpublicWow, took me quite a while to figure out this module is probably responsible why the original language node suddenly becomes access denied when I set the translated node to unpublished. What I cannot figure out: Is there a patch available that works? The latest patch in #33 only seems to include tests and doesn't solve the problem, right?
Comment #40
rgpublicAh, I found out: You have to use the -dev-Version. Then it's already fixed. Easy to do this wrong especially when you are using composer which readily installed the @alpha version for me. Perhaps there should be a new alpha release?
Comment #41
MegaChriz CreditAttribution: MegaChriz at WebCoo commented@rgpublic
I reflected in the issue summary that a fix is already in the dev version and that this issue is only open for adding test coverage.
Comment #42
rgpublic@MegaChriz: Ah, thanks - I missed that. Nevertheless: I still think a new version should be released, though - even without a test. This is soooo difficult to figure out because nothing gives you a hint, that this module could be responsible :-(
Comment #43
robertom CreditAttribution: robertom at bmeme commentedThis issue is solved with the dev version of this module, but if someone continue to have similar problems in the listings made with the views, check if this (core) issue can help #3061782: Add langcode from views filter to query metadata
Comment #44
DanielVezaAny chance we could roll a new release for this? We use this module in our distro and I'm not going to add a dev branch :(.
Otherwise how can I help with the tests to push this along? They seem to be in a good state - Is this missing a maintainer?
Comment #45
amaria CreditAttribution: amaria commentedThis is now in version rc1. However, the test is not yet included.
Comment #46
amaria CreditAttribution: amaria commentedPutting this back to Needs Review until the tests are committed
Comment #47
AmiOta CreditAttribution: AmiOta as a volunteer commentedI tested a fresh install of this module(8.x-1.0-rc2) in a Drupal 8.9.1 multilanguage(en/es) and when clicked in "rebuild permissions" all worked, it passes from "1 permissions in use" to "# permissions in use", no 403 errors.
Comment #48
amaria CreditAttribution: amaria commentedI think we’re way past the tests for this. It’s been 3 years, so I think I missed that window. :) I’ll create a different issue to address update testing in general, and link back to this. So we don’t hold this ticket open and confuse anyone. Thanks to all who helped!
Comment #49
amaria CreditAttribution: amaria commented