Task
Convert the following theme functions to use the new table #type:
Module | Theme function name | Where in Code | What is it really? |
---|---|---|---|
node | theme_node_search_admin | function | form table |
Changes in this patch
- theme_node_search_admin() is removed (along with the theme hook "node_search_admin").
- This theme function was only used for theming the content ranking section of the config page for the NodeSearch plugin in NodeSearch::buildConfigurationForm(). That function is refactored to use a regular #type = table render array instead of #theme = node_search_admin.
- In the process of doing that, the form elements for the weights of the rankings changed their HTML element names in the markup, so NodeSearch::submitConfigurationForm() also changed how it reads the ranking values for saving in the config, and SearchRankingTest also had to change how it submitted these forms.
- An Aria label is added to the ranking HTML selects, for accessibility.
Manual testing steps
- Enable the Search module
- Visit
admin/config/search/pages/manage/node_search
to review the generated HTML.
Beta phase evaluation
Issue category | Task because we only change the way the markup for an admin page is generated. |
---|---|
Issue priority | Major because parent meta issue #1876712: [meta] Convert all tables in core to new #type 'table' is major. |
Unfrozen changes | Unfrozen because it only changes the way markup is being printed |
Prioritized changes | This is not a prioritized change for the beta phase. |
Disruption | Non disruptive for core because we only change way markup is being printed. It also improves accessibility by providing aria-labels instead of hidden |
Related issues
Comment | File | Size | Author |
---|---|---|---|
#86 | interdiff.txt | 741 bytes | lauriii |
#86 | 1938920-convert-node-tables-85.patch | 7 KB | lauriii |
#68 | interdiff-1938920-67-68.txt | 706 bytes | akalata |
#68 | 1938920-convert-node-tables-68.patch | 7 KB | akalata |
#67 | interdiff-1938920-62-67.txt | 2.24 KB | akalata |
Comments
Comment #1
shanethehat CreditAttribution: shanethehat commentedComment #2
shanethehat CreditAttribution: shanethehat commentedI'm not sure if this one needs converting, at least not to the table type. The form is already using type 'detail', so converting it to table removes the detail markup. Is there a way to add the form as a child to the detail type that I'm missing?
Comment #3
shanethehat CreditAttribution: shanethehat commentedI've done an initial conversion, and the markup seems to match, especially the name attributes of the form elements. The form does not save the content ranking values, but I have tested the original form on a clean install, and that does not seem to save the values either.
The only differences I've seen in the markup are:
Before:
After:
Comment #4
shanethehat CreditAttribution: shanethehat commentedSame patch, with the right status
Comment #6
shanethehat CreditAttribution: shanethehat commented#4: convert-node-tables-1938920-3.patch queued for re-testing.
Comment #7
shanethehat CreditAttribution: shanethehat commentedHmm, so maybe this should be postponed until #1831632: Convert node_rank_ $rank variables to use configuration system lands, since that will change the way the rankings are retrieved in node_search_admin().
Comment #8
jibranTagging.
Comment #9
Jon PughThe data does not with patch in #4.
Comment #10
Jon PughComment #11
Jon PughI am re-rolling #1831632: Convert node_rank_ $rank variables to use configuration system with this patch at the same time, since they depend on one another.
Comment #12
Jon PughPatch queued for testing over at #1831632: Convert node_rank_ $rank variables to use configuration system contains the work done here. They are so closely tied it was much easier to include both in the same patch.
Comment #13
tstoecklerSo this should be duplicate then?
Comment #14
Petr IllekReopening this issue because the work in #1831632: Convert node_rank_ $rank variables to use configuration system was not actually doing anything for #1987406: node.module - Convert theme_ functions to Twig.
Comment #15
ianthomas_ukI'm working on #1831632: Convert node_rank_ $rank variables to use configuration system and hope to have a patch ready to commit within the next week or so. To avoid conflicting patches, please check the status of that issue before working on this one.
Comment #15.0
ianthomas_ukRemoving theme_node_recent_block()
Comment #16
xjm(Merging "node system" and "node.module" components for 8.x; disregard.)
Comment #17
star-szrLooks like this can probably be re-activated and that possibly this has been partially converted, current code:
So this might just be a matter of moving this #type table out into \Drupal\node\Plugin\Search\NodeSearch::buildConfigurationForm() and removing the theme function.
Comment #18
star-szrAnd tagging so we can keep track of this on http://drupaltwig.org.
Comment #19
martin107 CreditAttribution: martin107 commentedPatch no longer applies.
Comment #20
star-szrI don't think a reroll is applicable here.
Comment #21
Jon PughI'll start on this one again. #NYCcamp
Comment #22
Jon PughDone!
So simple... $type table assumes all element_children are rows, and element_children of that are columns.
Comment #23
Jon PughComment #25
Jon PughOk, should be all fixed up.
I refactored the naming of the forms to remain consistent with the config.
Comment #26
martin107 CreditAttribution: martin107 commentedtriggering testbot
Comment #28
Jon PughRerolled.
Comment #29
Jon PughAdding newline at end of file.
Comment #31
Jon PughAdjusting the test now.
Stand by for patch.
Comment #32
Jon PughUpdated the field identifiers, but there's an error happening that I can't figure out:
Comment #33
Jon PughComment #35
nicholasruunu CreditAttribution: nicholasruunu commentedI'll try to make the patch work.
Comment #36
nicholasruunu CreditAttribution: nicholasruunu commentedReroll patch
Comment #37
martin107 CreditAttribution: martin107 commentedTriggering testbot
Comment #39
akalata CreditAttribution: akalata commentedComment #40
akalata CreditAttribution: akalata commentedattempted re-roll due to PSR-4 filepath changes (I think?) #2083547: PSR-4: Putting it all together
Comment #42
akalata CreditAttribution: akalata commentedUpdating tests for search settings path change; also fixed error in reroll exposed by one of the failed tests.
Comment #43
martin107 CreditAttribution: martin107 commentedI wanted to see the interdiff....
So here it is... I'm glad to see so many issues being updated at TCDrupal
Comment #44
akalata CreditAttribution: akalata commentedThanks martin107! I thought I posted the interdiff , must've gotten lost from all of us posting at once. :)
I've got a few questions about why SearchRankingTest.php has hardcoded rankings to test for on admin/config/search/pages/manage/node_search, when 2 of those items aren't enabled in the Standard install profile, but I'd like to post this patch and interdiff to get the 4 other items to hopefully pass.
Comment #49
akalata CreditAttribution: akalata commentedIn an attempt to stop uploading fixes to testbot to see if they pass, I tried running the SearchRankingsTest testRankings locally. I'm getting the error below, even on a fresh HEAD:
I looked at #1919358: Tests failing on local dev environment with 8.x HEAD and clean DB. and checked my file permissions, but that doesn't appear to have any affect.
Comment #50
tim.plunkettComment #51
martin107 CreditAttribution: martin107 commentedreroll tag was stale
patch applied cleanly against head :-
commit d21c9a4e2e95477865cec3da1d3faad18d7cd5ae
Author: Alex Pott
Date: Sun Aug 10 17:38:02 2014 -0500
Issue #2313615 by tim.plunkett: Clarify the deprecated-ness of ArrayAccess usage in FormState.
Comment #52
martin107 CreditAttribution: martin107 commented@akalata
When I repeat your command
php core/scripts/run-tests.sh --class 'Drupal\search\Tests\SearchRankingTest'
I get the same result as you, but I suspect the problem is the command. Most notably when I include the --url to my site everything passes, no fails. :)
php core/scripts/run-tests.sh --php /Applications/MAMP/bin/php/php5.5.10/bin/php --color --die-on-fail --url http://dev.drupal.co.uk --verbose --file ./core/modules/search/src/Tests/SearchRankingTest.php
All the other options are a distraction, I think but am including them just for reference.
Comment #53
akalata CreditAttribution: akalata commentedComment #54
star-szrNo promises, but I'm going to try and figure this one out.
Comment #55
akalata CreditAttribution: akalata commentedTaking this over @ Fox Valley Drupal Camp sprint. I'm determined to get this done! Still no luck with running tests via command line, but I found the GUI. :)
Comment #56
akalata CreditAttribution: akalata commentedReroll #44.
While trying to fix testRankings() I realized that there's actually something wrong in submitConfigurationForm() from some earlier point. Working with @YesCT to debug that as well.
Comment #57
martin107 CreditAttribution: martin107 commentedTriggering testbot.
Comment #59
akalata CreditAttribution: akalata commentedThanks @martin, I knew it would fail which is why I didn't trigger it. :) Was just sharing my progress up to that point.
Here's another progress marker -- fixing the form submission (at admin/config/search/pages/manage/node_search), which was an error I made in an earlier re-roll. Now working on fixing the test.
Comment #60
akalata CreditAttribution: akalata commentedAnd I didn't think I'd get the tests working this quickly! It's amazing what hours spent figuring out docs and getting the debugger working will help you get done! :)
Comment #61
star-szrWoohoo green! Awesome work.
I don't think #type markup is needed. See https://api.drupal.org/api/drupal/developer%21topics%21forms_api_referen....
Comment #62
akalata CreditAttribution: akalata commentedA better link might be #2125043: Remove '#type' => 'markup' where all other instances were removed from D8 core. :) So in that case...
Comment #63
akalata CreditAttribution: akalata commentedComment #64
akalata CreditAttribution: akalata commentedComment #65
joelpittetHere's a code review from @a-fro and me. We are going to manual test this now!
This should be the injected t method because we can:) $this->t(). Also maybe we can use the short array syntax? []
Can we change these to the new array syntax too, not required obvious, but seems to be the preference.
Comment #66
joelpittetAfter we did manually testing we discovered the missing visually hidden label for the select element.
There was a hidden label for accessibility here, we can replace this with an 'aria-label' which @jessebeach suggested and I'd prefer(less markup).
Comment #67
akalata CreditAttribution: akalata commentedYay feedback!
Here are edits based on #65 and #66, plus the removal of node_search_admin from node_theme that I discovered after upgrading my local machine to PHP 5.5.
Comment #68
akalata CreditAttribution: akalata commentedBased on in-person feedback from @joelpittet, making the aria-label we added translatable.
Comment #69
joelpittetThank you @akalata and @a-fro, this looks great and still accessible!
Comment #70
YesCT CreditAttribution: YesCT commentedComment #71
akalata CreditAttribution: akalata commentedComment #72
akalata CreditAttribution: akalata commentedComment #73
akalata CreditAttribution: akalata commentedYesCT suggesting tagging with accessibility, and to mention that dawehner suggests that the conversion to #type=table is a security improvement, though I'm not familiar enough with the existing theme function to confirm.
Comment #74
mgiffordJust looking to the standards first for reference:
http://www.w3.org/TR/wai-aria/roles#tac_example2
Also other references:
http://msdn.microsoft.com/en-us/library/windows/desktop/jj658627%28v=vs....
This looks fine when comparing it to those references.
Note: Tested in VoiceOver & ChromeVox. Works as expected.
Comment #75
joelpittetThank you @mgifford for the accessibility test!
Comment #76
alexpottthe aria-label of "Influence of Number of comments" has interesting capitalisation.
Also I don't think that this will work for translation.
Comment #77
joelpittet@mgifford what do you think, should we lowercase the title or just remove the prefix extra 'Influence of '?
Comment #78
jhodgdonI was asked to comment here... since I just came to this issue, it's a bit confusing. The issue summary just says it's trying to get rid of this custom theme function, but the patch seems to be doing a number of other changes that are not explained in the summary or code comments. Maybe an issue summary update would be useful?
Regarding the aria label... It seems like the right fix would be to make the labels something like:
Influence of "Number of comments"
rather than removing the "Influence of" part.
Comment #79
joelpittetThank you @jhodgdon, that sounds like a good idea
That change is so we don't have to create a label and have lighter markup and better accessibility.
The other changes that seem evident are just key changes due to the structure of the #type table. It's kind of hacky either way, you change the keys or you change #parents to fake the keys you expect... it's a toss up in my opinion, do you have a preference?
Comment #80
jhodgdonIt seems like less markup and fewer levels of key parents is a good idea.
Comment #81
akalata CreditAttribution: akalata commentedI've added single quotes around the ranking variable name, since I was worried that double quotes would affect the
aria-label=""
.I've also tested these labels in multiple interface languages, and I'm not seeing any issues with the @title substitution.
I didn't quite follow the conversation in 79/80, so I didn't change any keys.
Comment #82
akalata CreditAttribution: akalata commentedComment #84
jhodgdonI updated the issue summary and added a section explaining the changes I found in this patch. Please check it over. After looking at this, I think I'd actually be more comfortable with leaving the HTML markup for the input elements as it was. There is at least one contrib module that extends this plugin (Display Suite) and I think it would just be better at this stage not to change the HTML markup for the form element used for rankings. This could affect contrib modules and their tests, and I don't think the change is necessary for this patch.
Also, one small nitpick:
When you make a string with ' inside it, it is much better to use
"... 'something' ..."
than' ... \'something\'...
. So this line should be changed. It's more readable and also that's our coding standard.Comment #85
jhodgdonI'm also changing the component to search.module. Normally everything related to Core Search goes in that component. As you can see, all the tests are in that module, and people check that component for search-related stuff.
Comment #86
lauriiiTested this manually and everything seems to work. This looks good to me.
Comment #87
jhodgdonI disagree with the RTBC. I do not think at this stage we should change the element names in the HTML markup for the select lists. This patch could achieve its aims without making that change. See #84.
Comment #88
star-szrFrom what I recall working on/reviewing other #type table issues there is little to no control over what name and id attributes you end up with, they are automatically generated based on the array structure and the -value is automatically appended to the ID. So the slight changes to markup have usually been accepted as part of the conversion process. I can't find the code (or chain of code) right now that is responsible for those changes.
This is RTBC in my opinion but I don't want to start that game. It's worth mentioning that markup is not frozen until RC1: https://www.drupal.org/core/beta-changes#unfrozen
Comment #89
star-szrOr put another way: the markup here is not being changed intentionally, it's changing as a result of converting to #type table.
Comment #90
jhodgdonReally?
It looks to me as though it used to be $form['content_ranking']['factors']['rankings_$var'] and now it's $form['content_ranking']['rankings'][$var]['value'] . Both are hard-wired, it's just hard-wired differently now.
This is not just markup; it's also affecting the value structure of $form_state.
Comment #91
star-szrI'm not sure if it's possible to preserve the $form_state structure within the constraints of #type table but I'm happy to be proven wrong :)
Comment #92
jhodgdonOh, I see... I was not understanding the reason for the markup change. OK. I'm OK with it now. Sorry for that!
So, before this can be committed:
- We need a draft change record
- We need the "beta policy" section in the issue summary updated. See https://www.drupal.org/contribute/core/beta-changes -- and I don't think it's actually true that there is no disruption, as that section currently states, because the markup has changed and $form_state, so potentially modules extending the NodeSearch class could be affected.
Comment #93
alexpottThis issue is a normal task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.
Also #92 points out we need a CR.
Comment #94
jhodgdonOh sorry, I should not have set it to RTBC without those items that are outlined in #93/#93 being present.
Comment #95
lauriiiComment #96
lauriiiComment #97
YesCT CreditAttribution: YesCT commentedadding remaining tasks section to the summary (with link to how to draft a change record)
Comment #98
jhodgdonFixed missing end of phrase in beta eval section in summary.
Still needs draft change record.
Comment #99
joelpittetAdded a draft change record. Hopefully that covers enough?
https://www.drupal.org/node/2385285
Comment #100
jhodgdonLooks pretty good. I tweaked the wording a bit.
All tasks have been done now, so back to RTBC. Thanks!
Comment #101
joelpittetVery nice tweaking:) Thanks.
Comment #102
alexpottCommitted d07e48a and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation for to the issue summary.