Improved template comment parsing logic and moved to separate class.
Details
Details
- Reviewers
alex
Preparations - make following changes in system files:
- add test settings to supportedSettings property in core/units/helpers/template_comment_parser.php:
- 'PRIORITY' => array('type' => 'integer', 'default' => 1),
- 'INCLUDE_IN_SITEMAP' => array('type' => 'boolean', 'default' => false),
- add test code to the core/admin_templates/themes/file_edit.tpl, after <div id="scroll_container"> line:
- <?php
- global $application;
- $object = $application->recallObject('theme-file');
- echo '<pre>';
- print_r(unserialize($object->GetDBField('FileMetaInfo')));
- echo '</pre>';
- ?>
Part 1 - test that non-existing template produces exception
- change code of core/units/helpers/themes_helper.php add
- $file_path = 'no_such_file'; before $this->_getTemplateMetaInfo(trim($file_path, '/'), $theme_id) call in the FindThemeFiles method
- go to Configuration → Website → Themes
- press Rescan Themes button
- confirm, that exception like Template file "/web/5.2.x/themes/advanced/no_such_file" not found. happens.
- restore code of core/units/helpers/themes_helper.php method, changed above
Part 2 - test that template without meta comment get default values from template comment parser
- go to Configuration → Website → Themes
- press Rescan Themes button
- open advanced theme to edit
- go to Files tab
- open some template, that have no meta comment (for example, one from action_box.elm.tpl templates) to edit
- confirm, that no meta tag is visible in editor under Description label
- confirm, that test output on edit page contains:
- [name] =>
- [desc] =>
- [section] =>
- [priority] => 1
- [include_in_sitemap] =>
Part 3 - test that template with one line long meta comment get default values from template comment parser
- add test meta comment <!--## ##--> to the /in-news/elements/side_boxes/action_box.elm.tpl template
- go to Configuration → Website → Themes
- press Rescan Themes button
- open advanced theme to edit
- go to Files tab
- open template /in-news/elements/side_boxes/action_box.elm.tpl to edit
- confirm, that added meta comment is visible in editor under Description label
- confirm, that test output on edit page contains:
- [name] =>
- [desc] =>
- [section] =>
- [priority] => 1
- [include_in_sitemap] =>
Part 4 - test that unsupported setting produces exception
- change test setting in the meta comment of the `platform/login/forgot_password.tpl' template:
- <PRIORITI>5</PRIORITI>
- go to Configuration → Website → Themes
- press Rescan Themes button
- confirm, that exception like Meta-comment in "/web/5.2.x/themes/advanced/platform/login/forgot_password.tpl" template file contains unsupported setting "PRIORITI". Supported settings are: "NAME", "DESC", "SECTION", "PRIORITY", "INCLUDE_IN_SITEMAP". happens.
Part 5 - test that unknown format of setting value produces exception
- change test setting in the meta comment of the `platform/login/forgot_password.tpl' template:
- <PRIORITY>top</PRIORITY>
- go to Configuration → Website → Themes
- press Rescan Themes button
- confirm, that exception like Only positive numbers are allowed as "PRIORITY" setting value in "/web/5.2.x/themes/advanced/platform/login/forgot_password.tpl" template file. happens.
- change test setting in the meta comment of the `platform/login/forgot_password.tpl' template:
- <PRIORITY>-5</PRIORITY>
- go to Configuration → Website → Themes
- press Rescan Themes button
- confirm, that exception like Only positive numbers are allowed as "PRIORITY" setting value in "/web/5.2.x/themes/advanced/platform/login/forgot_password.tpl" template file. happens.
- change test settings in the meta comment of the `platform/login/forgot_password.tpl' template:
- <PRIORITY>5</PRIORITY>
- <INCLUDE_IN_SITEMAP>true</INCLUDE_IN_SITEMAP>
- go to Configuration → Website → Themes
- press Rescan Themes button
- confirm, that exception like Only "yes" or "no" are allowed as "INCLUDE_IN_SITEMAP" setting value in "/web/5.2.x/themes/advanced/platform/login/forgot_password.tpl" template file. happens.
Part 6 - test regular output when all settings values are valid
- change test settings in the meta comment of the `platform/login/forgot_password.tpl' template:
- <NAME>Forgot Password</NAME>
- <DESC>test description</DESC>
- <SECTION>My Account||Login</SECTION>
- <PRIORITY>5</PRIORITY>
- <INCLUDE_IN_SITEMAP>yes</INCLUDE_IN_SITEMAP>
- go to Configuration → Website → Themes
- press Rescan Themes button
- open advanced theme to edit
- go to Files tab
- open template /platform/login/forgot_password.tpl to edit
- confirm, that added meta comment is visible in editor under Description label
- confirm, that test output on edit page contains:
- [name] => Forgot Password
- [desc] => test description
- [section] => My Account||Login
- [priority] => 5
- [include_in_sitemap] => 1
Diff Detail
Diff Detail
- Repository
- rINP In-Portal
- Branch
- /in-portal/branches/5.2.x
- Lint
Lint Errors Excuse: Full file reformatting is not part of this task. Severity Location Code Message Error core/units/helpers/themes_helper.php:455 PHPCS.E.Generic.WhiteSpace.ScopeIndent.IncorrectExact Generic.WhiteSpace.ScopeIndent.IncorrectExact - Unit
No Unit Test Coverage - Build Status
Buildable 614 Build 614: arc lint + arc unit
Event Timeline
Comment Actions
Adding comment to trigger automatic link creation between Phabricator and JIRA (was broken after last upgrade of Phabricator).
Comment Actions
Today looks that made all necessary changes, but not full-tested yet. Left part of tests planned on next day.
Comment Actions
Haven't tested code yet, but here are needed corrections.
| core/kernel/application.php | ||
|---|---|---|
| 736 | remove this line no real reason for this class to be placed in core/kernel folder | |
| core/kernel/utility/template_comment_parser.php | ||
| 1 |
no real reason for this class to be placed in core/kernel folder | |
| 2–15 | Please use same broken formatting for licensing comment as in other files. As I've mentioned in other differential revision licensing comments would be mass-fixed later on. | |
| 3–4 | remove this there should be no description in file licensing comment | |
| 7 | please update year to 2016 the end year in license should match year, when this file is comitted | |
| 54 | add missing @throws tags There should be separate @throws tag for each unique thrown exception, but this one only matches code: throw new Exception($exception_message_start . 'invalid XML in the comment.'); | |
| 58–61 | move this to the constructor we shouldn't be allowing to create parser from non-existing file in the first place | |
| 88–90 | Please use SimpleXML extension to do all XML-related stuff. The SimpleXML extension was used in D120 from where this code was (or should have been) extracted. Any solid reason for not continue use of SimpleXML extension? | |
| 91 |
throw new Exception(sprintf( 'Template file "%s" has unsupported setting "%s".', $this->templateFile, $setting )); extracting part of error message creates grammatically incorrect error messages as a result, e.g. Template file "xxx.tpl" have must have integer value in setting "yyy". | |
| 115–116 | replace empty line + break with break + empty line CS | |
| 122 | move setting word in all exception messages to the end grammatically the "xxx" setting is correct, but setting "xxx" is incorrect | |
| core/units/theme_files/theme_file_eh.php | ||
| 124 | inline this variable variable isn't used only once | |
Comment Actions
Some of requested changes might need changing the test plan as well.
| core/units/helpers/template_comment_parser.php | ||
|---|---|---|
| 36–37 ↗ | (On Diff #721) | Please remove. These 2 settings look as ones in your test plan. |
| 76–81 ↗ | (On Diff #721) | Please transform this into block comment (not DocBlock): /* * Template starts with comment in such format: * <!--## * <NAME></NAME> * <DESC></DESC> * <SECTION>||</SECTION> * ##--> */ |
| 86 ↗ | (On Diff #721) | Please replace with: throw new Exception(sprintf( 'Unclosed meta-comment in "%s" template file.', $this->templateFile )); Error message now is more user friendly. |
| 99 ↗ | (On Diff #721) | Remove. Doesn't really shed more light on error causes. |
| 100 ↗ | (On Diff #721) | Please replace with: throw new Exception(sprintf( 'Meta-comment syntax error in "%s" template file.', $this->templateFile )); Error message now is more user friendly. |
| 108 ↗ | (On Diff #721) | Please replace with: 'Meta-comment in "%1$s" template file contains unsupported setting "%2$s". Supported settings are: %3$s.', Error message now is more user friendly. |
| 110 ↗ | (On Diff #721) | Replace with: $setting_name, '"' . implode('", "', array_keys($this->supportedSettings)) . '"' Error message now is more user friendly. |
| 115 ↗ | (On Diff #721) | Please:
Array key access for validation is removed (micro-optimization). |
| 129 ↗ | (On Diff #721) | Please replace with: 'Only positive numbers are allowed as "%2$s" setting value in "%1$s" template file.', Error message now is more user friendly. |
| 135–141 ↗ | (On Diff #721) | Please combine this and above checks into same IF sharing same error message. The error message was updated to reflect both cases. |
| 147 ↗ | (On Diff #721) | Please replace with: 'Only "yes" or "no" are allowed as "%2$s" setting value in "%1$s" template file.', Error message now is more user friendly. |
| core/units/helpers/template_comment_parser.php | ||
|---|---|---|
| 3 ↗ | (On Diff #724) |
The $Id$ property isn't up to date. |
| 17 ↗ | (On Diff #724) | Add final keyword to class declaration. |
| 92 ↗ | (On Diff #724) |
Solves BC break, that cause XML parsing error for meta comments using & instead of &. |
| 109 ↗ | (On Diff #724) |
$setting_name = $setting->getName(); $setting_value = trim($setting);
|
| 111–152 ↗ | (On Diff #724) | Move out into parseSettingValue($name, $value) method, that will return $result_value variable. |
| 120 ↗ | (On Diff #724) | This variable would only be needed after extracted method call. |
| 121 ↗ | (On Diff #724) | When code around this place would be extracted into new method the initial value of this variable would come from method parameters. |
| 133 ↗ | (On Diff #724) | Please replace $parsed < 1 with $parsed < 0. The 0 is considered as positive number as well. |
| core/units/helpers/themes_helper.php | ||
| 452–454 |
Avoids BC break when method is removed. P.S. | |
| core/units/theme_files/theme_file_eh.php | ||
| 121–128 |
Prevents BC break (see previous comments). | |
| core/units/helpers/template_comment_parser.php | ||
|---|---|---|
| 92 ↗ | (On Diff #724) | Actually don't do this. |
| 98–108 ↗ | (On Diff #724) | Please revert back to preg_match_all approach used in kThemesHelper currently: if ( preg_match_all('/<([^>]*)>(.*?)<\/([^>]*)>/is', $comment, $regs) ) { $ret = array(); foreach ( $regs[1] as $param_order => $setting_name ) { $setting_value = trim($regs[2][$param_order]); // call the "parseSettingValue" method } } Attempt to interpret template meta comment as XML turned out to be bad, because all invalid XML (e.g. & instead of & and such) is now causing issues. |