Currently the check to see if a role ID is valid is done in the menu access callbacks for TAXONOMY_ACCESS_CONFIG . '/role/%/edit'
and TAXONOMY_ACCESS_CONFIG . '/role/%/delete'
. This might be confusing, because it implies there's some actual content the user is allowed to access, when actually there's no content.
Instead, we should move these checks to page callbacks and return MENU_NOT_FOUND instead of FALSE
. In fact, the access callbacks can probably be removed entirely and replaced with 'access arguments' => array('administer permissions')
. The page callbacks would then return drupal_get_form('formname, args).
Comment | File | Size | Author |
---|---|---|---|
#17 | return_404_invalid_term_IDs-1366168-17.patch | 8.15 KB | sammyd56 |
#17 | interdiff.txt | 2.67 KB | sammyd56 |
#12 | return_404_invalid_term_IDs-1366168-12.patch | 7.92 KB | sammyd56 |
#12 | interdiff-10-12-return_404_invalid_term_IDs-1366168.patch | 3.89 KB | sammyd56 |
#10 | return_404_invalid_term_IDs-1366168-10.patch | 4.03 KB | sammyd56 |
Comments
Comment #1
xjmFor consistency, the page callbacks should be named
taxonomy_access_formname_page()
, and be intaxonomy_access.admin.inc
.Comment #2
sammyd56 CreditAttribution: sammyd56 commentedI've had a go at getting my head around this, but haven't had much success as I'm not able to pass the variables correctly...
Am I on the right track at least?
Comment #3
xjmThis looks like a good start. It's not immediately obvious to me why you're getting that error messsage; the menu hook looks correct at first glance. I'll take a closer look later, but in the meanwhile, here's a couple other things to fix:
Let's change this to "Page callback: Returns the role configuration form."
These two parts can be removed, because the access arguments will take care of that check.
Let's add this same comment to the same section in taxonomy_access_delete_role_page().
For the second argument in these two function calls, it should be $rid rather than 5. (5 is just the number of the path component, and it will get passed in as $rid).
Similarly, let's add a docblock here that has the summary, "Page callback: Returns the role deletion confirmation form."
Let's add a comment above this line that says "Do not allow access control to be disabled for the anonymous or authenticated user roles."
This line should also return MENU_NOT_FOUND rather than FALSE.
Finally, for the two page callbacks, let's add the path, the @param for rid, and the @see to our menu hook implementation. (See http://drupal.org/node/1354#menu-callback for an example.)
Thanks!
Comment #4
xjmOh--be sure to clear your site cache and reload once or twice each time you test a change.
Comment #5
sammyd56 CreditAttribution: sammyd56 commentedUpdated as per #3, appears to be working fine now.
Comment #6
sammyd56 CreditAttribution: sammyd56 commentedOops, messed up the first comment block! This should be better...
Comment #7
xjmCool, this looks correct now. Few more minor cleanup points:
These look good, but we should add the prefix
Path:
in front of them.These should actually be
taxonomy_access_menu()
. :)There's a few extra linebreaks here and there. (And possibly elsewhere in the module, but development is too active right now to clean other lines up.) For the lines added in the patch, there should be:
{
and}
and the function contents. (I prefer a linebreak before an inline comment generally, but they can be omitted at the top of the function.)}
and the start of the next function's docblock.Also, I think maybe we should put
$rid = intval($rid);
at the top of these, to sanitize the input immediately.Comment #8
sammyd56 CreditAttribution: sammyd56 commentedHere's an updated patch. Sorry, have been trying to keep on top of coding standards but it's a lot to keep track of when you're starting out. Thanks for the pointers.
Comment #9
xjmCool, this looks great. I'll test it locally when I have a chance. If all goes well then we can add some automated tests as well, though I haven't thought through entirely everyhing they should test.
Comment #10
sammyd56 CreditAttribution: sammyd56 commentedOops, forgot to add an in-line comment in the second page callback.
In terms of tests, I guess we need to:
Anything I've missed?
Comment #11
xjmExcellent, #10 covers it I think. For testing invalid roles, I'd try both numeric role IDs that don't exist, like 0 and something really big, 43993; and also a string (
$this->randomName()
).Comment #12
sammyd56 CreditAttribution: sammyd56 commentedHere's a first attempt at adding the tests. They seem to be working OK. Interdiff (#10-#12) included.
Comment #14
sammyd56 CreditAttribution: sammyd56 commentedI've opened a new issue for the exceptions: #1376064: Undefined index notices in role/%/edit title callback
Comment #15
xjmHmm, didn't expect that the title callback would be called before the page callback, but I guess it makes sense. I'd actually prefer to fix #1376064: Undefined index notices in role/%/edit title callback here, since it's not an issue if the access callback handles the request. How about this to fix those notices:
Comment #16
xjmOh, minor thing, these comments should have articles and periods, e.g.:
// Try a string.
Also, I recommend giving interdiffs the extension
.txt
so that testbot doesn't try to test them. :) (Although, in this case, the interdiff was the same thing as the test-only patch to expose the fail, so that's good.) Thanks!Comment #17
sammyd56 CreditAttribution: sammyd56 commentedUpdated patch
Comment #17.0
sammyd56 CreditAttribution: sammyd56 commentedUpdated issue summary.
Comment #18
rudolfbykerThis seems to work just fine.
Comment #21
nravens CreditAttribution: nravens commentedThis still seems to be an issue with the latest release. I'm using 7.x-1.0 and noticed that users who don't have permission to view content tagged with a TAC taxo get a 404 page not found message instead of a 403.
Is this fixed in the current dev version?