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.Part of meta-issue #1310084: [meta] API documentation cleanup sprint
| Comment | File | Size | Author |
|---|---|---|---|
| #14 | doc-cleanup-themes-1431630-14-D7.patch | 15.64 KB | NROTC_Webmaster |
| #12 | doc-cleanup-themes-1431630-12.patch | 16.12 KB | NROTC_Webmaster |
| #12 | theme-interdiff.txt | 5.11 KB | NROTC_Webmaster |
| #6 | drupal-1431630-6.patch | 15.79 KB | tim.plunkett |
| #5 | drupal-1431630-5.patch | 0 bytes | tim.plunkett |
Comments
Comment #1
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedFirst attempt. The themes seem to be horribly documented and I'm a little uncertain even how some of the files should be handled. I tried to give fairly generic descriptions because I don't really use the themes.
Comment #2
jhodgdonHmmm... This looks like a good start. I reviewed the Bartik changes, and have these comments:
a) I am not sure what those .html files do, but I don't think we should add PHP tags to them, so I think we should not have @file in there. Eek! Too scary to contemplate. :)
b) I would also not change this line in color/preview.js:
c) For the template.php file, I think a better @file description would be:
Functions to support theming in the Bartik theme.
d)
Actually that should all be in the @file. See
http://drupal.org/node/1354#templates
e) The same goes for the other .tpl.php files. They all need to be modified to fit that standard.
Comment #3
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedThanks for the quick response. The stark template.php says
* @todo Based on outcome of http://drupal.org/node/1471382, revise this
* technique to use conditional classes vs. conditional stylesheets.
Since that issue is closed (won't fix) should I remove it from the docblock?
Other than that I have updated the patch to comply with the documentation.
Comment #4
jhodgdonFYI - When putting links to issues on drupal.org, use the [#...] syntax, where ... is the issue's node ID. Makes nicer links. :)
Regarding that @todo - that is beyond the scope of this issue, and that other issue has several comments indicating there might be a follow-up, so I would just leave it alone.
Anyway... thanks, and someone (probably xjm or I) will try to review this sometime soon. :)
Comment #5
tim.plunkettReroll due to a single line removed in #1419968: Replace $('selector', domelement) with $(domelement).find('selector').
Comment #6
tim.plunkettderp.
Comment #7
filijonka CreditAttribution: filijonka commentedin the template.php (all themes)
all functions named hook_preprocess_hook or hook_process_hook should be documented as hooks, see [#1354-hookimpl]
Comment #8
jhodgdonRE #7 - that is true if the hooks are listed at http://api.drupal.org/api/drupal/core!includes!theme.inc/function/theme/8
For instance, a function called bartik_preprocess_html should be documented as:
Implements hook_preprocess_HOOK() for html.tpl.php.
(thereby telling us which HOOK it is for).
Comment #9
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedJust to be sure, the following functions should have that type of documentation?
Most of these currently say "Overrides or inserts ..." but should instead say what they are implementing.
drupal\core\themes\bartik\template.php
Line 33: function bartik_process_html(&$variables) {
Line 43: function bartik_process_page(&$variables) {
Line 94: function bartik_process_maintenance_page(&$variables) {
drupal\core\themes\bartik\template.php
Line 11: function bartik_preprocess_html(&$variables) {
Line 112: function bartik_preprocess_block(&$variables) {
drupal\core\themes\seven\template.php
Line 11: function seven_preprocess_maintenance_page(&$vars) {
Line 23: function seven_preprocess_html(&$vars) {
Line 31: function seven_preprocess_page(&$vars) {
drupal\core\themes\stark\template.php
Line 14: function stark_preprocess_html(&$variables) {
Comment #10
tim.plunkettThe existing lines could be left in, but they are often redundant.
If left in, it should look like this:
Comment #11
jhodgdonI would take out a line like the second one in #10. As stated there, it's redundant.
Comment #12
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedThe latest is attached with the diff from #6.
Comment #13
jhodgdonThis patch looks good, and I think it's safe to commit even though we're over thresholds (doesn't seem to conflict with any of the critical/major issue patches in the queue). So, I committed it to 8.x! Thanks!
Next up: port to D7.
Comment #14
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedThis one seems fairly straightforward but since the theme standard was added for D8 I will refer to your best judgement.
Comment #15
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedThis one seems fairly straightforward but since the theme standard was added for D8 I will refer to your best judgement.
Comment #16
jhodgdonThis looks pretty good -- I think it is fine to use the hook_preprocess_HOOK() doc standard in D7, because the alternative is linking to functions that do not exist (i.e., won't make links).
I did notice this:
Did we do that in d8? We should not have. The @ingroup themeable should only be on the base theme implementation, not in overrides of it in individual themes.
... Yes, it looks like we did that in the D8 patch. Let's go back and fix that. All of those @ingroup themeable lines need to be taken out of all the themes' tpl.php files.
Other than that, the D7 patch looks good.
Comment #17
jhodgdonAnd I just clarified that on http://drupal.org/node/1354#templates
Comment #18
jhodgdonThese issues are a lot of work with very little tangible payoff, so I'm closing the rest of them as "won't fix". Your efforts on working on this issue were appreciated... it was just my fault for starting a task that was very difficult to get right.
Let's instead put our effort into fixing and reviewing documentation that is really unclear and/or wrong, and I hope that the people who worked on these issues are not afraid to jump into a more reasonable issue!