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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
FileSize
5.69 KB
9.46 KB
alexpott’s picture

FileSize
785 bytes
9.67 KB
alexpott’s picture

+++ b/view_unpublished.install
@@ -17,3 +17,16 @@ function view_unpublished_install() {
+/**
+ * Fix multilingual grants by rebuilding node access permissions.
+ */
+function view_unpublished_update_8001(&$sandbox) {
+  _node_access_rebuild_batch_operation($sandbox);
+  // Populate the update sandbox as expected.
+  $sandbox['#finished'] = $sandbox['sandbox']['progress'] / $sandbox['sandbox']['max'];
+  if ($sandbox['#finished'] == 1) {
+    node_access_needs_rebuild(FALSE);
+  }
+  return t('The content access permissions have been rebuilt.');
+}

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

alexpott’s picture

alexpott’s picture

FileSize
610.6 KB
619.16 KB

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

alexpott’s picture

FileSize
263 bytes
12.41 KB

Noticed the test name and file name were not aligned.

amateescu’s picture

Status: Needs review » Needs work

Looks like the latest patch doesn't include the database dump anymore..

+++ b/view_unpublished.install
@@ -17,3 +17,17 @@ function view_unpublished_install() {
+    [':rebuild' => \Drupal::url('node.configure_rebuild_confirm')]

We can use Url::fromRoute()->toString() here :)

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.74 KB
619.43 KB

Thanks @amateescu

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Looks ready to me :)

drupalninja99’s picture

Status: Reviewed & tested by the community » Needs work

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

JeroenT’s picture

Status: Needs work » Reviewed & tested by the community

Tried this module and it worked for me.

flyke’s picture

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

ayalon’s picture

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

ayalon’s picture

I 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

AND ( EXISTS  (SELECT na.nid AS nid
FROM 
node_access na
WHERE (( (gid IN  ('0')) AND (realm = 'all') )OR( (gid IN  ('125')) AND (realm = 'view_unpublished_author') )OR( (gid IN  ('1')) AND (realm = 'view_unpublished_published_content') )OR( (gid IN  ('1')) AND (realm = 'view_unpublished_content') ))AND (na.grant_view >= '1') AND (na.fallback = '1') AND (node_field_data.nid = na.nid) )) 

Second access filter

AND ( EXISTS  (SELECT na.nid AS nid
FROM 
node_access na
WHERE (( (gid IN  ('0')) AND (realm = 'all') )OR( (gid IN  ('125')) AND (realm = 'view_unpublished_author') )OR( (gid IN  ('1')) AND (realm = 'view_unpublished_published_content') )OR( (gid IN  ('1')) AND (realm = 'view_unpublished_content') ))AND (na.grant_view >= '1') AND (na.fallback = '1') AND (node_field_data_node__field_facility_ref.nid = na.nid) )) 

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

  • amaria committed a8db1b5 on 8.x-1.x authored by alexpott
    Issue #2784887 by alexpott, amateescu, ayalon, drupalninja99, JeroenT,...
amaria’s picture

Status: Reviewed & tested by the community » Fixed

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

alexpott’s picture

@amaria you can now enable automated testing on the project because there's a test!

amaria’s picture

Oh right. Thanks. Actually, there was a test already I just hadn't enabled the automated testing.

  • amaria committed aea97e8 on 8.x-1.x
    Issue #2784887: Removing failing tests.
    
amaria’s picture

Status: Fixed » Needs work

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

hoebekewim’s picture

FileSize
1.03 KB

Re-roll to match the current dev version.
Sorry for the missing interdiff.

achap’s picture

The patch in #22 failed testing with this message:

PHP Fatal error:  Cannot redeclare view_unpublished_update_8001() (previously declared in /var/www/html/modules/contrib/view_unpublished/view_unpublished.install:31) in /var/www/html/modules/contrib/view_unpublished/view_unpublished.install on line 50
xargs: php: exited with status 255; aborting

Is it possible that the patch you created didn't remove the previous declaration?

achap’s picture

FileSize
8.84 KB

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

achap’s picture

FileSize
615.62 KB

Attempt to fix this error message "error: cannot apply binary patch to 'tests/fixtures/view_unpublished_pre_8001.php.gz' without full index line"

achap’s picture

FileSize
8.63 KB

One more try, with fixture provided by core. Tests still passing locally.

achap’s picture

FileSize
8.49 KB

Used standard install profile, fixed spelling mistake.

achap’s picture

FileSize
196.33 KB
achap’s picture

FileSize
196.34 KB
achap’s picture

FileSize
208.88 KB
achap’s picture

Apologies 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:

testUpdatedSite
fail: [Other] Line 39 of modules/contrib/view_unpublished/src/Tests/Update8001Test.php:
Link containing href /subdirectory/admin/reports/status/rebuild found. 

Not a very helpful message.

Then it has another error (presumably because the first failure meant no nodes were created):

Drupal\Tests\view_unpublished\Functional\ViewUnpublishedMultilingualTest::testIt
Behat\Mink\Exception\ExpectationException: Current response status code is 404, but 403 expected.

Does anyone know why it's failing on the the assertLinkByHref() ?

achap’s picture

FileSize
8.91 KB

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

achap’s picture

FileSize
209.07 KB
achap’s picture

Status: Needs work » Reviewed & tested by the community

The last submitted patch, 2: 2784887.2.patch, failed testing. View results

The last submitted patch, 3: 2784887.3.patch, failed testing. View results

achap’s picture

Status: Reviewed & tested by the community » Needs review
kruser’s picture

Yep, this is still broke.

rgpublic’s picture

Wow, 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?

rgpublic’s picture

Ah, 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?

MegaChriz’s picture

Issue summary: View changes

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

rgpublic’s picture

@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 :-(

robertom’s picture

This 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

DanielVeza’s picture

Any 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?

amaria’s picture

Status: Needs review » Fixed

This is now in version rc1. However, the test is not yet included.

amaria’s picture

Status: Fixed » Needs review

Putting this back to Needs Review until the tests are committed

AmiOta’s picture

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

amaria’s picture

Status: Needs review » Fixed

I 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!

amaria’s picture

Related issues: +#3161472: Add update tests

  • amaria committed 1c1f569 on 8.x-1.x
    Issue #2784887 by achap, alexpott, hoebekewim, amaria, amateescu, ayalon...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.