Problem/Motivation
When creating a custom menu there isn't an option to link the menu to a content type.
I had the idea to make a custom menu where I could add some basic pages.
The steps I had in mind:
1) Create the menu
2) Create a basic page
3) When creating the page add it to my custom menu
I got stuck on 3 because I couldn't find the menu and I didn't had a clue where to start looking. Untill someone told me I had to edit the content type to allow the new custom menu. This is confusing and there are no hints in the UI you have to do this.
Proposed resolution
Add checkboxes containing the available content types when creating/editing the menu
Remaining tasks
- Current re-roll includes edits to admin page callbacks that no longer exist in Drupal 8.x, so the patch has no effect
- Need to integrate UI changes into the code that generates the menu creation/edit pages in Drupal 8.x
- If creating a new Menu module, needs to have an .info.yml file included
- Maybe change the description of the form element
User interface changes
A new checkboxes form element linking a menu to a content type, or multiple content types.
API changes
No API changes
Comment | File | Size | Author |
---|---|---|---|
#92 | 1239666-nr-bot.txt | 144 bytes | needs-review-queue-bot |
#90 | 1239666-90.patch | 2.69 KB | Charchil Khandelwal |
#89 | 1239666-89.patch | 2.69 KB | Charchil Khandelwal |
#89 | before-patch.PNG | 40.35 KB | Charchil Khandelwal |
#89 | after-patch.PNG | 42.17 KB | Charchil Khandelwal |
Comments
Comment #1
Bojhan CreditAttribution: Bojhan commentedDon't we just have checkboxes? I could see that work here too.
Comment #2
aspilicious CreditAttribution: aspilicious commentedHere is a proof of concept patch. It hurted my brain due to the upside down logic of fetching and saving the values.
I don't know if this is the right way to do this. So be gentle ;)
Comment #3
xjmI tested the patch locally, and it functions as expected:
Obviously, some user instructions would go here. :)
Instead of
node_type_get_names()
, can we just use the form definition here? Edit: Just during submission, I mean.Needs capitalization and a period.
Ditto.
I'd say either remove this comment, or make it more descriptive.
Comment #4
aspilicious CreditAttribution: aspilicious commentedThank you for your review. I fixed all of them. Probably needs a better description but I leave that to someone else :).
I attached a screenshot for people that do not understand what this is all about.
Comment #5
aspilicious CreditAttribution: aspilicious commentedChanging title to reflect the state of the issue
Comment #6
sunThe other way around: Node has to integrate with Menu.
Comment #7
sunComment #7.0
sunSmall edits
Comment #8
aspilicious CreditAttribution: aspilicious commentedIn irc we decided this is the correct approach after all. We aldo agreed these checkboxes should also be visible on menu creation, like I planned to do in the beginning.
New patch...
Comment #9
sun1) No need to check this on menu creation.
2) Could use an inline comment that explains what is being done, as the operation is a bit mind-boggling.
Missing space after if.
Let's name this 'menu_node_types'.
Replace #title with #description and drop the #description.
No need to call this twice.
Like above, this operation is equally not "obvious", so could use an inline comment explaining what is being done.
This in_array() looks superfluous to me.
Lastly, we need tests for this feature.
25 days to next Drupal core point release.
Comment #10
aspilicious CreditAttribution: aspilicious commentedBack with a new patch:
Note: I spent 3 hours writing these tests... Nobody is ever going to hire me :p
Comment #11
aspilicious CreditAttribution: aspilicious commentedNew patch and screenshot!
Comment #12
xjmThe patch looks good to me, and this seems like a useful parity to have in the UI.
The UI text here is a little weird. Not sure what to change it to, at least not before my coffee. ;)
I'd suggest changing this to "Verify this menu is listed on the article administration form." and "Disable the menu from the article administration page." (With periods.)
Comment #13
xjmUntagging because the feature has tests.
Comment #14
aspilicious CreditAttribution: aspilicious commentedbetter comments, maybe needs a better description...
Comment #15
sunThis entire code needs to be wrapped in a module_exists('node').
Especially due to that, I do not think this feature belongs into Menu module, but instead, Node module should integrate with Menu module.
Both modules are optional. But the actual configuration is primarily used on the node form.
I understand that menu.module already contains integration code in the other direction, but that looks equally bogus to me, now that Node module is optional. At the very least, I'd like to see a follow-up issue to move all of the integration code from Menu to Node module.
We need to explain _why_ the add form case is ignored.
1) No semicolon at the end.
2) Could be shortened.
Please double-check your git configuration to create patches with -p to provide more context in diff hunks. (There should be a menu_yadayada_form_submit() context reference on this line.)
Drop the first part "Check if we need to" without replacement.
Comment #16
yoroy CreditAttribution: yoroy commentedSuggestion for the label: "Make this menu available for these content types"
Comment #17
Bojhan CreditAttribution: Bojhan commentedOr even "Available for these content types" you are already in the activity, of "creating this menu".
The general concept is sound, I remember this being quite awkward when we designed it - as it was a one-way solution.
Comment #18
xjmComment #19
ryan.gibson CreditAttribution: ryan.gibson commentedI'll take this one.
Comment #20
ryan.gibson CreditAttribution: ryan.gibson commentedComment #21
ryan.gibson CreditAttribution: ryan.gibson commentedComment #23
xjmThis needs parens around the condition.
Really nitpicky, but can we be consistent between these two?
Let's strip the t() from these assertion messages. Reference: http://drupal.org/simpletest-tutorial-drupal7#t
Comment #24
trrroy CreditAttribution: trrroy commentedComment #25
trrroy CreditAttribution: trrroy commentedpatch updated and resubmitted for test
Comment #26
xjmThanks @trrroy! Setting to "needs review" so the patch can be tested by the testbot and reviewed by reviewers.
So, unrelated. Neither aspilicious nor I can remember why we decided with sun that menu should integrate with node rather than vice versa. aspilicious thinks it might have had to do with not being able to get the correct
$menu
array in node. (We really should have documented it...) As far as aspilicious recalls, the three of us came to the conclusion that it had to be this way, and sun mentioned a followup issue to decouple menu from node. aspilicious didn't know if that issue actually exists.Tagging for a summary update to straighten this out, and also for research to find a followup issue for further decoupling node and menu.
Comment #28
sunThe key needs to be in double-quotes, as it contains a $variable you want to be expanded.
The variable expansion is the reason why double-quotes are slightly slower, and thus, why we generally use single quotes everywhere, unless double quotes are needed.
That said, the surrounding code in this test uses single quotes with string concatenation of $type. Beautiful code is consistent, so please decide on one way :) (I think I'd personally go with double quotes here)
Comment #29
xjmNo, really.
Comment #30
trrroy CreditAttribution: trrroy commentedChanging to double quotes.
Comment #31
sunI'm fine with this patch. Only code comments could use some love - the added code in the form has no explanation at all for what is being done there - and why.
Likewise, the code in the form submit handler accesses a lot of variables without checking whether they actually exist. That said, the test code should be moved into an own test case, and ideally, the test case verify the correct behavior with both Node module being disabled and enabled.
Lastly, there's a very minor coding style error in the test (missing space in array declaration).
Comment #32
trrroy CreditAttribution: trrroy commentedI added the comments to form additions and fixed the missing space in array declaration. Still need to check for variables in the submit handler and split out the test case (from comment in 31). I'm just getting my feet wet with test cases, please bear with me.
Comment #33
trrroy CreditAttribution: trrroy commentedok, variables are checked and a new test case is broken out with the node module disabled. I'm getting some warnings from simpletest about "call_user_func_array() expects parameter 1 to be a valid callback, function '_node_add_access' not found or invalid function name" when trying to access 'admin/structure/menu/manage/$menu_name/add'. I'm not sure how to get rid of those.
Comment #35
trrroy CreditAttribution: trrroy commentedComment #36
xjmThanks @trrroy! I'm working with irunflower during office hours this morning, and she's going to add some code style cleanups to your patch as follows:
Trailing whitespace on all these lines should be removed.
TRUE should be all in caps, the sentence should begin with a capital and end with a period, and there should be articles (e.g. "if the callback function does not exist"). Also, be sure to wrap this comment to 80 chars when it's updated.
I'd say "when creating or updating a menu." Also, the comment needs to end in a period.
Needs capitalization and a period.
This needs to be 80 characters or less and begin with a third-person verb. Also, can we clarify a little what the test is testing?
This comment is over 80 characters long and should be wrapped earlier.
These docblocks all have superfluous blank lines after the summary, and should begin with a third-person verb (e.g. "Tests"). Also, the docs should be a little more specific (describing what the tests are doing).
Can we clarify this comment more?
When submitting a new patch, please include an interdiff so @trrroy can easily see the changes we're making.
Comment #37
irunflower CreditAttribution: irunflower commentedUpdated.
In #36, comment #1, the last part of trailing white space - couldn't find trailing white space in:
In #6, comments #7 and #8 still have to be addressed.
Comment #38
irunflower CreditAttribution: irunflower commentedInterdiff made. First time making an interdiff, but I checked the comparisons between the patches, and I think I made this correctly...
Comment #39
aspilicious CreditAttribution: aspilicious commentedTrailing whitespaces
You got a lot of trailing whitespaces in your patch.
You should configure your editor so it removes them automaticly.
12 days to next Drupal core point release.
Comment #40
trrroy CreditAttribution: trrroy commentedThanks @irunflower! I'm new to interdiffs too so I'm not certain but I don't think that #38 had all your changes in it. I pulled from 37, checked whitespaces and completed 6/7/8 from #36. I'm pretty sure its all in here so far.
Comment #41
trrroy CreditAttribution: trrroy commentedComment #42
trrroy CreditAttribution: trrroy commentedoops, I found more whitespace. try this again.
Comment #43
xjmThis is looking a lot better! Thanks @irunflower and @trrroy.
I've reviewed the latest patch and noticed the following:
Uh. Stray debug.
An explanation of "why" would be good in this comment.
I think some of comments are wrapping too early? They should go as close to 80 char as possible without going over. Can you double-check?
I may have said this earlier--while this is gross, decoupling menu from node is a significant task and beyond the scope of this issue, so this is acceptable in that sense.
Missing period. Also, there should be articles (e.g., "Abort if the menu values do not exist."
This is a fairly useless error message...
These docblocks should begin with a one-line summary that is 80 characters or less and begins with a third-person verb. Reference: http://drupal.org/node/1354#functions
These changes, while (in most cases) correct by our coding standards, are out of scope for the issue. When code style errors exist elsewhere in core, we should fix them in followup issues (e.g. #1326672: Clean up API docs for menu module). In this patch, we should only make sure that new or changed lines comply with the standards, and not change other lines. See: http://xjm.drupalgardens.com/blog/core-mentoring-and-xjms-guide-patch-re...
This comment could be clarified. "Maps to"?
This API change also needs to include an update to the function's docblock to indicate that $item_node is optional.
We can omit
t()
from the assertion message here (the final argument). Reference: http://drupal.org/simpletest-tutorial-drupal7#tAlso, let's add a period to it.
These should begin with "Tests" rather than "Test". Also, these are the extra blank lines that should be removed, because they are lines we are adding in this patch.
When creating a patch, please read the patch file before uploading to make sure it includes the intended changes. Thanks!
Comment #44
trrroy CreditAttribution: trrroy commentedAll comments from 43 addressed. On #9, I'm not sure if my change answers the question (also, I'm not sure if I understand the code/comment). On #10, should that docblock be re-formatted with descriptions below the parameters?
Comment #45
trrroy CreditAttribution: trrroy commentedComment #46
aspilicious CreditAttribution: aspilicious commentedTrailiing whitespace
Trailing whitespace
You should rly look at automated tools to remove whitespace in your editor.
It can save you a lot of trouble! :)
(I had the same whitespace issues in the past)
7 days to next Drupal core point release.
Comment #47
trrroy CreditAttribution: trrroy commented(arg! there was whitespace in 44! sorry! I thought I checked that.) Please check my questions in comment #44.
Comment #48
sunheh. You got rid of those by hacking menu.inc? LOL! But hey, at least I like your creativity :)
All patches since #35 are carrying on this hack, which obviously needs to be removed.
Also, the additional test methods in MenuTestCase have to be moved into an own, new test case class (in case there is no test case for testing the node/menu associations already; I suspect there's probably one in node.test already).
Comment #49
trrroy CreditAttribution: trrroy commentedOk, I removed my hacks and looked for an existing test for node/menu associations but didn't see one. I did move the new MenuNodeDisabledTest into its own test case but I'm back to the same error that I had before that I'm not sure how to get rid of "call_user_func_array() expects parameter 1 to be a valid callback, function '_node_add_access' not found or invalid function name." Any suggestions?
Comment #50
trrroy CreditAttribution: trrroy commentedComment #52
trrroy CreditAttribution: trrroy commentedComment #52.0
trrroy CreditAttribution: trrroy commentedFixed mistake
Comment #53
xjm(Merging "node system" and "node.module" components for 8.x; disregard.)
Comment #54
dcmul CreditAttribution: dcmul as a volunteer commentedA lot of work was put into this one, but its seems like such a long time. Adding tag Needs reroll.
Comment #55
Usha Gavva CreditAttribution: Usha Gavva as a volunteer commentedRerolled the patch from Comment #49
Comment #56
siva_epari CreditAttribution: siva_epari as a volunteer commentedPlease don't remove tags.
Comment #57
jgeryk CreditAttribution: jgeryk commentedComment #58
vaidehi bapat CreditAttribution: vaidehi bapat at TATA Consultancy Services for Pfizer, Inc. commentedWorking on this issue as a part of Drupal Con Asia 2016 code sprint.
Comment #59
vaidehi bapat CreditAttribution: vaidehi bapat at TATA Consultancy Services for Pfizer, Inc. commentedI tried applying patch provided in #55 on latest 8.0.x version. The patch gets applied. However, it doesn't work as expected. No content type links provided while creating or editing menu.
Comment #60
vaidehi bapat CreditAttribution: vaidehi bapat at TATA Consultancy Services for Pfizer, Inc. commentedComment #61
vaidehi bapat CreditAttribution: vaidehi bapat at TATA Consultancy Services for Pfizer, Inc. commentedComment #66
mayurjadhav CreditAttribution: mayurjadhav at Blisstering Solutions commentedComment #67
nitesh624Comment #68
nitesh624Comment #70
mohit1604 CreditAttribution: mohit1604 at Google Summer of Code commentedI applied patch #55 in my local environment but it does not make any UI changes !
Comment #71
jennine CreditAttribution: jennine commentedI'm at DrupalCon Nashville sprints and I'm going to start looking into this issue!
Comment #72
jennine CreditAttribution: jennine commentedComment #73
jennine CreditAttribution: jennine commentedTagging this as Needs Reroll again because the reroll in #55 was not sufficient. It replicates the menu.admin.inc file from Drupal 7.x, but the callbacks in that file are no longer present in Drupal 8.x. That's why applying the patch has no effect. Also, since the Menu core module does not exist in Drupal 8.x, #55 would require that module to be installed, which is not possible because there is no .info.yml file.
The patch needs to be rerolled in such a way that it fits into the way menus are managed in Drupal 8.x Core.
Comment #74
savkaviktor16@gmail.com CreditAttribution: savkaviktor16@gmail.com at Skilld commentedI've redone the main part of the patch #49 meaning out of tests according to way menus are managed in Drupal 8.x Core
That's why I didn't make interdiff. I'm not good at creating unit tests, so would be great if someone more experienced will take care of it .
Comment #76
aleevasThe last patch was rerolled successfully.
Solved 2 conflicts in
core/modules/menu_ui/src/MenuForm.php b/core/modules/menu_ui/src/MenuForm.php
Comment #78
AjitSThe patch still applies
Comment #83
Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commentedA re-rolled patch against 9.3.x. Please review.
Comment #84
chetanbharambe CreditAttribution: chetanbharambe at QED42 for Drupal India Association commentedHi @Suresh Prabhu Parkala,
Above test cases, #83 is getting failed.
Can you please check it once again and attached some before and after screenshots.
Needs to be re-rolled for 9.3.x
Can be a move to Needs work
Comment #85
ankithashettyFixed custom command failure errors in #83, thanks!
Comment #86
vikashsoni CreditAttribution: vikashsoni as a volunteer and at Zyxware Technologies commentedI have applied patch #85 in durpal-9.3.x-dev after patch we can configure menu for specific content type for ref sharing screenshot
Comment #89
Charchil Khandelwal CreditAttribution: Charchil Khandelwal at Dotsquares Ltd. commentedPatch No. #85 tested and applied successfully on drupal 9.4.x version.
Comment #90
Charchil Khandelwal CreditAttribution: Charchil Khandelwal at Dotsquares Ltd. commentedComment #92
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.