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.Problem/Motivation
Working on a new feature for the Devel module: #2289899: Show additional Views query information on the live preview area, it was discovered that the second argument for hook_views_preview_info_alter was not passed correctly. Instead of it being a ViewExecutable, it is passed the instance of ViewsUI (this contains the ViewExecutable inside it, which is unusable by any hook implementation due to it being a protected property).
See: https://api.drupal.org/api/drupal/core%21modules%21views%21views.api.php...
This hook should also have a test for it, particularly as it is not currently implemented in core.
Proposed resolution
Fix the argument being passed to the hook, and write a test to ensure the hook is invoked correctly.
To test the hook we need it to be implemented somewhere in code first. As we don't have this in core anywhere, a new test module needs to be created (which will be called views_ui_test). It is therefore proposed that we use SimpleTest, rather than PHPUnit. This way we can enable a test module, that implements hook_views_preview_info_alter.
Remaining tasks
Fix the argument being passed through to hook_views_preview_info_alterWrite a test to ensure the hook is invoked correctly.Create test-only patch, to ensure this fails.Run both test-only and full patch through the testbot again, and check this runs okay against latest HEAD.
User interface changes
None
API changes
None
Original report by [leon.nk]
hook_views_preview_info_alter is documented (in views.api.php) to accept a ViewExecutable as a second argument. Also see documentation here.
However, in ViewsUI.php it is actually passed the instance of ViewsUI, and not the executable inside it.
Comments
Comment #1
Leon Kessler CreditAttribution: Leon Kessler commentedPatch attached.
Comment #2
Leon Kessler CreditAttribution: Leon Kessler commentedComment #3
dawehnerOh
Comment #4
alexpottWe should take the opportunity to add tests for
hook_views_preview_info_alter()since it is completely unused in code.Comment #5
Leon Kessler CreditAttribution: Leon Kessler commentedOkay here's a first attempt with a test.
I had to create a new module to implement
hook_views_preview_info_alter. I gave it a generic name of views_ui_test, so although it only contains one function, it could be used for other stuff later.I added the test to PreviewTest.php, see patch attached.
Comment #6
Leon Kessler CreditAttribution: Leon Kessler commentedAdded
views_ui_testtwice to the modules array. Fixed in new patch.Comment #7
Leon Kessler CreditAttribution: Leon Kessler commentedStill some issues with the patch (the string argument is not passed correctly to xpath()).
Also, do we want to add some additional tests for the views preview info area? Such as one for the execute time.
Comment #8
Leon Kessler CreditAttribution: Leon Kessler commentedOkay this one should be good, new patch...
Comment #9
Leon Kessler CreditAttribution: Leon Kessler commentedComment #10
nlisgo CreditAttribution: nlisgo commentedI am going to review this patch
Comment #11
nlisgo CreditAttribution: nlisgo commentedSpecifically I will be sanity checking the code only for the moment and providing feedback on the approach.
Comment #12
jOksanen CreditAttribution: jOksanen commentedManually tested this patch.
1. Patched my local setup with the patch
2. Ran the patch with and without the fix in the patch (notice without the fix)
3. Patch tested all clear
Comment #13
Sutharsan CreditAttribution: Sutharsan commented@nlisgo
please assign an issue to yourself while working on it. This will prevent others to start working on it.Ignore my comment. We have been instructed not to assign here during DrupalCon sprint.
Comment #14
Leon Kessler CreditAttribution: Leon Kessler commentedComment #15
nlisgo CreditAttribution: nlisgo commentedI am struggling to find a similar scenario in core where a hook was provided in core and there is a test for that hook but there is no use for that hook in core.
Comment #16
Finn Lewis CreditAttribution: Finn Lewis commentedAfter a few local issue, I too have applied the patch and run the tests.
Patch applies cleanly and the suite of tests in DRUPAL\VIEWS_UI\TESTS\PREVIEWTEST all pass.
Removing the original patch then gives the expected notice
"Undefined property: Drupal\views_ui\ViewUI::$result"
Let me know if there are any other tasks on this I can help with.
Comment #17
Leon Kessler CreditAttribution: Leon Kessler commentedComment #18
dawehnerIt totally makes sense to have that additional test and fixing the passing. Great work.
Comment #19
nlisgo CreditAttribution: nlisgo commentedEven though the issue is now RTBC, I think we should run it through the testbot as it has been 3 months since it was last run.
I've reviewed the patch and read every line and have found that the patch introduces the necessary fix and test for the outlined issue and does not introduce fixes that are out of scope.
I'm also uploading a test-only patch which I should expect to fail and then a patch which includes the fix to core/modules/views_ui/src/ViewUI.php which should pass.
I have made 2 minor changes to the previous patch:
Comment #20
nlisgo CreditAttribution: nlisgo commentedThe provided test only patch failed as expected and the re-rolled patch including the fix passed as expected.
Please review.
Comment #22
Leon Kessler CreditAttribution: Leon Kessler commentedGreat! Thanks nslingo.
Test-only patch fails as expected.
Your changes to original patch look good.
All remaining tasks are now complete, this now just needs a review and set to RTBC.
Comment #23
nlisgo CreditAttribution: nlisgo commentedThe testbot automatically switched this issue to needs work because the test-only patch failed. I am switching this to needs review as the re-rolled patch passed: https://www.drupal.org/files/issues/2293899-hook-views-preview-info-pass...
Comment #24
dawehnerAwesome!
Comment #25
alexpottCommitted 062ca31 and pushed to 8.0.x. Thanks!