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
admin/reports/upgrade doesn't correctly redirect. If views is enabled, it should use URL arguments instead of setting the session. See #2904549: Remove routing.yml for admin/reports/upgrade for why we are keeping both.
Proposed resolution
Let's keep the menu item, and fix it to use url query parameters if views is around and loosen up the security.
Remaining tasks
Code it.
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#72 | 2904546-70.patch | 4.13 KB | alexpott |
#72 | 67-70-interdiff.txt | 655 bytes | alexpott |
#67 | interdiff_62-67.txt | 1.49 KB | sarvjeetsingh |
#67 | 2904546-67.patch | 4.13 KB | sarvjeetsingh |
#62 | interdiff-2904546-58-62.txt | 4.29 KB | msuthars |
Comments
Comment #2
heddnThis is tied to the upgrade log report. That seems a little restrictive. Perhaps we should use the same permission already in use for dblog, which is 'access site reports'?
return AccessResultAllowed::allowedIf((int) $account->id() === 1)->mergeCacheMaxAge(0);
Second issue:
This might have worked when dblog was still using the '/admin/reports/dblog' and \Drupal\dblog\Controller\DbLogController::overview menu item. But it got converted to a view and this no longer is functional. Instead of this session logic, we can use admin/reports/dblog?type%5B%5D=migrate_drupal_ui
Comment #3
heddnComment #4
MaskOta CreditAttribution: MaskOta commentedComment #5
heddnThis actually needs to handle with views and without views. So, let's add a module handler and check if the views is enabled and use one set of arguments. Or the previous arguments if views isn't enabled.
Comment #6
MaskOta CreditAttribution: MaskOta commentedThis should do it then. Thanks for reviewing
Comment #7
heddnI think you still need the custom access check. That route will get overwritten by views, so no need to do anything special with the permissions if views is enabled. Otherwise, this looks pretty good.
Comment #8
MaskOta CreditAttribution: MaskOta commentedI didn't remove the custom access check. I just replaced the one on the reports route with the permission to access site reports. Which seems consistent with the other report routes to me.
Comment #9
heddnLet's not change that permissions check though.
Comment #10
dev.patrick CreditAttribution: dev.patrick at TATA Consultancy Services for Pfizer, Inc. commentedAdding to patch#6 chaining permission and custom_access.
Comment #11
dev.patrick CreditAttribution: dev.patrick at TATA Consultancy Services for Pfizer, Inc. commentedComment #12
quietone CreditAttribution: quietone at Acro Commerce commented@dev.patrick, thanks for making the patch.
Whitespace error. If not aligned.
Comment #13
dev.patrick CreditAttribution: dev.patrick at TATA Consultancy Services for Pfizer, Inc. commentedThanks @Quietone, corrected white space error.
Comment #14
quietone CreditAttribution: quietone at Acro Commerce commented@dev.patrick, your welcome. But...
the pesky whitespace moved to here!
Comment #15
dev.patrick CreditAttribution: dev.patrick at TATA Consultancy Services for Pfizer, Inc. commentedMy bad. Using windows and has some issue with IDE. Really appreciate if you give another chance.
Comment #16
maxocub CreditAttribution: maxocub as a volunteer commentedThis looks good. I tested it manually and it works. We should add automated testing, that shouldn't be too hard.
Comment #17
Ivan Berezhnov CreditAttribution: Ivan Berezhnov as a volunteer and at Drupal Ukraine Community for Levi9 commentedComment #20
Chi CreditAttribution: Chi commentedThis is rather tricky. Even if Views module exists it does not guarantee that the Watchdog view is in place. Users are free to remove/modify any views.
Comment #22
jfurnas CreditAttribution: jfurnas commentedWould adding another conditional for checking if the view/display is available and enabled help?
Comment #23
heddnProbably that would help.
Comment #24
mrweiner CreditAttribution: mrweiner as a volunteer commentedAdded checks to patch from #15 to make sure that:
Comment #25
mrweiner CreditAttribution: mrweiner as a volunteer commentedSetting to "Needs Review"
Comment #26
heddnThis is looking really nice. Any chance for a functional test with and without that view enabled? NW for tests.
Comment #27
mrweiner CreditAttribution: mrweiner as a volunteer commentedDo you have any tips on how to structure such a test? I haven't written any tests before.
Comment #28
heddnhttps://www.drupal.org/docs/8/phpunit/phpunit-browser-test-tutorial has some details. You can also look at other functional tests in core or google for other tutorials.
Comment #29
mrweiner CreditAttribution: mrweiner as a volunteer commentedAlright, new patch includes tests, fixes a couple of minor errors, and removes the redundant permission check on the route since it's handled by
\Drupal\migrate_drupal_ui\MigrateAccessCheck::checkAccess
Comment #30
mrweiner CreditAttribution: mrweiner as a volunteer commentedComment #31
mrweiner CreditAttribution: mrweiner as a volunteer commentedCoding standards
Comment #32
mrweiner CreditAttribution: mrweiner as a volunteer commentedAlright one more try! Removing whitespace
Comment #33
mrweiner CreditAttribution: mrweiner as a volunteer commentedAdding test group -- I think this should do it.
Comment #34
heddnThanks for your contribution. Wonderful to see you figured out testing. A few small points to consider below...
This could be combined into a
!empty
check.This should be protected I think.
I don't see any asserts in these 2 tests that we are actually seeing filtered results. Which is sorta the intent of this issue.
Let's duplicate the code from the test above here. Less coupling in tests is usually better.
Comment #35
vacho CreditAttribution: vacho at Skilld commentedSome refactor over #33 patch that solve points 1, 2, 4 about #34.
I don't understand well this point please more details:
"3. I don't see any asserts in these 2 tests that we are actually seeing filtered results. Which is sorta the intent of this issue. "
Comment #36
vacho CreditAttribution: vacho at Skilld commentedComment #37
heddnLet's use the class path for the file.
Here we're testing that we redirect to the view if it is enabled. But we don't have test a for if the view isn't enabled. Could we add a test of the url if the view is disabled too?
Comment #38
mrweiner CreditAttribution: mrweiner as a volunteer commentedI think we are getting a bit jumbled up, here. Don't have my machine atm but here are some notes.
You're referencing the assertion in testRedirectDestinationWithoutView() which is the test without the view enabled -- it's being deleted prior to asserting the url. Did you mean to say that we need to add a test with the view enabled?
@vacho, why'd you remove testRedirectDestination()? It could be renamed to testRedirectDestinationWithView(), I suppose, but we need two separate tests in here.
Comment #41
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedChanges done according to #37.
@heddn Please review.
Comment #42
quietone CreditAttribution: quietone commentedJust looking at the documentation.
Sentence doesn't span 80 columns correctly and is missing a full stop.
Method needs a summary line.
Missing summary line
s/Click/Clicks/
Missing return type
Line > 80 characters.
Needs a datatype
Comment #43
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedDone changes according to #42,
@quietone, Please review.
Comment #44
quietone CreditAttribution: quietone commentedAssigning to myself for review and testing
Comment #45
quietone CreditAttribution: quietone commented@vsujeetkumar, thanks for continuing with this.
I think this is missing a test. There are currently two tests, one with the view disabled and one with the view deleted. There needs to be a test with the view enabled.
During manual testing and running MigrateControllerTest the return statement was never reached. Perhaps my testing was not thorough but the tests certainly should be.
This is testing the Migrate Controller but it isn't a base class.
assertResponse() and assertUrl() are deprecated, Lets use whatever the recommended replacements are.
This method is used once which makes we wonder if it is really needed. Plus the return value is not used.
Comment #46
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedDone the changes advised in #45,
1: Added the test for enable view.
2: updated the class summery.
3: Fixed the deprecated assertResponse() and assertUrl() issues.
4: Rename the method "clickViewsDisableLink" to clickViewsOperationsLink and now it is used in multiple places, Also it is required for disable and enable the view.
@quietone Please review.
Comment #47
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedPlease ignore the previous interdiff(interdiff_43-46.patch), I have added correct interdiff.
Comment #48
quietone CreditAttribution: quietone commentedThanks for the fixes.
This time I looked closer at comments.
The summary line and the summary are virtually identical. This is a simple test I think a summary line is sufficient. Maybe 'Tests the upgrade path with the watchdog view disabled and enabled.' The summary line should follow the coding standards, Drupal API documentation standards for functions
clickViewsOperationsLink returns a value, why is that not used?
Can the address be a constant?
The last page navigated to was 'admin/reports/upgrade' which does not have an 'Enable' button. Am I missing something?
It looks like this is supposed to be one sentence, not two. It can be simpler, 'Tests redirection to report page when the watchdog view is enabled.'
Same here the summary line and summary are virtually the same.
Is this a sufficient test? Wouldn't it be better to test for ' The view Watchdog has been deleted.'
Should be one sentence and wrapped correctly.
s/Click/Clicks/
The summary says that this method will 'perform an operation' yet here it specifically says it is a disable link. Which is correct?
I've reread your comment and I see you changed the name of this method. Why is a name change needed here?
Needs a return type.
Not wrapped correctly and change 'onFailure' to 'on failure'. Would be good to add what causes a failure. I don't see that this returns page content, either. Oh, I see, it used to return a boolean TRUE or FALSE. What is the reason for changing that?
Comment #49
quietone CreditAttribution: quietone commentedHere is a patch for the points in #48.
Comment #51
quietone CreditAttribution: quietone commentedOops, forgot to uncomment a line in the test.
Comment #53
quietone CreditAttribution: quietone commentedRandom test failure, Drupal\Tests\quickedit\FunctionalJavascript\QuickEditIntegrationTest. Setting back to NR.
Comment #54
heddnLooks good now. Thanks for plugging away at this.
Comment #56
quietone CreditAttribution: quietone commentedThe failure occurs since #3164686: WebAssert::addressEquals() and AssertLegacyTrait::assertUrl() fail to check the querystring was committed. I have updated the test to reflect that. Tested manually as well.
I'm thinking that this can be replaced with
If views is not installed,
$this->config('views.view.watchdog')->get('status')
returns NULL. This seems a much simpler way to determine if the watchdog view is available.Comment #57
heddnI agree w/ the change in #56 for status. Did a quick check if we need to worry about a non-existent views config, but it seems that is handled already by config factory, so we're good. This is ready to go back to RTBC, except for a quick nit.
Unused import.
Comment #58
abhisekmazumdarThank you @quietone for the patch. I have updated the #56 patch as suggested by @heddn.
Comment #59
heddnComment #60
alexpottI don't think hardcoding config IDs here is correct. I don't think we should do the if at all. We can do something like:
I think we should be creating a message for testing purposes here so we can test that the type is selected - something like:
I don't think we should have 2 tests here. It's not necessary. We should also be testing the whether or not the correct type is selected rather than the URL. I.e. we should be testing what the user wants to see - they don';t care whether it is session or a query parameter is what makes it work.
So something like:
Comment #61
msutharsWorking on it.
Comment #62
msutharsI have updated the #58 patch as suggested by @alexpott.
Comment #64
msutharsComment #65
quietone CreditAttribution: quietone commentedI reviewed the patch and the changes suggested in #60 have been made. There are no coding standard errors. So, I though this was ready but then I noticed two problems with the comments.
This sentence doesn't make sense. Maybe something like 'Set both the session and query parameter so both the watchdog view and the fallback work correctly.' I am not sure
I think reports should be singular here, or am I wrong? s/reports/report/.
Comment #66
sarvjeetsingh CreditAttribution: sarvjeetsingh as a volunteer and at QED42 for Drupal India Association commentedComment #67
sarvjeetsingh CreditAttribution: sarvjeetsingh as a volunteer and at QED42 for Drupal India Association commentedMade changes as per the above suggestions. please review.
Comment #68
heddnLooks good now.
Comment #69
alexpottCommitted and pushed 0a8bf19ac6 to 9.1.x and 4190ccb3d8 to 9.0.x. Thanks!
I think this worth backporting to 8.9.x as it helps people with migrations.
Comment #72
alexpottRemoving the void.
Comment #73
alexpottCommitted 606eae5 and pushed to 8.9.x. Thanks!