Page MenuHomeIn-Portal Phabricator
Differential D220

INP-1553 Move out template meta comment parsing to TemplateCommentParser class
Needs ReviewPublic
Actions

Authored by erik on Apr 18 2016, 4:37 AM.

Details

Reviewers
alex
Summary

Improved template comment parsing logic and moved to separate class.

Test Plan

Preparations - make following changes in system files:

  1. 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),
  2. 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

  1. 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
  2. go to Configuration → Website → Themes
  3. press Rescan Themes button
  4. confirm, that exception like Template file "/web/5.2.x/themes/advanced/no_such_file" not found. happens.
  5. 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

  1. go to Configuration → Website → Themes
  2. press Rescan Themes button
  3. open advanced theme to edit
  4. go to Files tab
  5. open some template, that have no meta comment (for example, one from action_box.elm.tpl templates) to edit
  6. confirm, that no meta tag is visible in editor under Description label
  7. 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

  1. add test meta comment <!--## ##--> to the /in-news/elements/side_boxes/action_box.elm.tpl template
  2. go to Configuration → Website → Themes
  3. press Rescan Themes button
  4. open advanced theme to edit
  5. go to Files tab
  6. open template /in-news/elements/side_boxes/action_box.elm.tpl to edit
  7. confirm, that added meta comment is visible in editor under Description label
  8. confirm, that test output on edit page contains:
    • [name] =>
    • [desc] =>
    • [section] =>
    • [priority] => 1
    • [include_in_sitemap] =>

Part 4 - test that unsupported setting produces exception

  1. change test setting in the meta comment of the `platform/login/forgot_password.tpl' template:
    • <PRIORITI>5</PRIORITI>
  2. go to Configuration → Website → Themes
  3. press Rescan Themes button
  4. 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

  1. change test setting in the meta comment of the `platform/login/forgot_password.tpl' template:
    • <PRIORITY>top</PRIORITY>
  2. go to Configuration → Website → Themes
  3. press Rescan Themes button
  4. 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.
  5. change test setting in the meta comment of the `platform/login/forgot_password.tpl' template:
    • <PRIORITY>-5</PRIORITY>
  6. go to Configuration → Website → Themes
  7. press Rescan Themes button
  8. 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.
  9. 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>
  10. go to Configuration → Website → Themes
  11. press Rescan Themes button
  12. 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

  1. 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>
  2. go to Configuration → Website → Themes
  3. press Rescan Themes button
  4. open advanced theme to edit
  5. go to Files tab
  6. open template /platform/login/forgot_password.tpl to edit
  7. confirm, that added meta comment is visible in editor under Description label
  8. 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

Repository
rINP In-Portal
Branch
/in-portal/branches/5.2.x
Lint
Lint ErrorsExcuse: Full files re-formatting is not part of this task.
SeverityLocationCodeMessage
Errorcore/units/helpers/helpers_config.php:80PHPCS.E.CodingStandard.Array.Array.SpaceAfterKeywordCodingStandard.Array.Array.SpaceAfterKeyword
Errorcore/units/helpers/helpers_config.php:80PHPCS.E.Generic.PHP.LowerCaseKeyword.FoundGeneric.PHP.LowerCaseKeyword.Found
Errorcore/units/helpers/template_comment_parser.php:2PHPCS.E.CodingStandard.Commenting.DocComment.MissingShortCodingStandard.Commenting.DocComment.MissingShort
Errorcore/units/helpers/template_comment_parser.php:3PHPCS.E.Squiz.Commenting.DocCommentAlignment.SpaceBeforeStarSquiz.Commenting.DocCommentAlignment.SpaceBeforeStar
Errorcore/units/helpers/template_comment_parser.php:4PHPCS.E.Squiz.Commenting.DocCommentAlignment.SpaceBeforeStarSquiz.Commenting.DocCommentAlignment.SpaceBeforeStar
Errorcore/units/helpers/template_comment_parser.php:5PHPCS.E.Squiz.Commenting.DocCommentAlignment.SpaceBeforeStarSquiz.Commenting.DocCommentAlignment.SpaceBeforeStar
Errorcore/units/helpers/template_comment_parser.php:6PHPCS.E.Squiz.Commenting.DocCommentAlignment.SpaceBeforeStarSquiz.Commenting.DocCommentAlignment.SpaceBeforeStar
Errorcore/units/helpers/template_comment_parser.php:7PHPCS.E.Squiz.Commenting.DocCommentAlignment.SpaceBeforeStarSquiz.Commenting.DocCommentAlignment.SpaceBeforeStar
Errorcore/units/helpers/template_comment_parser.php:8PHPCS.E.Squiz.Commenting.DocCommentAlignment.SpaceBeforeStarSquiz.Commenting.DocCommentAlignment.SpaceBeforeStar
Errorcore/units/helpers/template_comment_parser.php:9PHPCS.E.Squiz.Commenting.DocCommentAlignment.SpaceBeforeStarSquiz.Commenting.DocCommentAlignment.SpaceBeforeStar
Errorcore/units/helpers/template_comment_parser.php:10PHPCS.E.Squiz.Commenting.DocCommentAlignment.SpaceBeforeStarSquiz.Commenting.DocCommentAlignment.SpaceBeforeStar
Errorcore/units/helpers/template_comment_parser.php:11PHPCS.E.Squiz.Commenting.DocCommentAlignment.SpaceBeforeStarSquiz.Commenting.DocCommentAlignment.SpaceBeforeStar
Errorcore/units/helpers/template_comment_parser.php:12PHPCS.E.Squiz.Commenting.DocCommentAlignment.SpaceBeforeStarSquiz.Commenting.DocCommentAlignment.SpaceBeforeStar
Errorcore/units/helpers/template_comment_parser.php:13PHPCS.E.Squiz.Commenting.DocCommentAlignment.SpaceBeforeStarSquiz.Commenting.DocCommentAlignment.SpaceBeforeStar
Errorcore/units/helpers/themes_helper.php:463PHPCS.E.Generic.WhiteSpace.ScopeIndent.IncorrectExactGeneric.WhiteSpace.ScopeIndent.IncorrectExact
Errorcore/units/helpers/themes_helper.php:469PHPCS.E.Generic.WhiteSpace.ScopeIndent.IncorrectExactGeneric.WhiteSpace.ScopeIndent.IncorrectExact
Unit
No Unit Test Coverage
Build Status
Buildable 815
Build 815: arc lint + arc unit

Event Timeline

erik updated this revision to Diff 520.Apr 18 2016, 4:37 AM
erik retitled this revision from to INP-1553 Move out template meta comment parsing to TemplateCommentParser class.
erik updated this object.
erik edited the test plan for this revision. (Show Details)
erik added a reviewer: alex.
erik added 1 JIRA issue(s): INP-1553.
alex edited edge metadata.May 5 2016, 6:24 AM

Adding comment to trigger automatic link creation between Phabricator and JIRA (was broken after last upgrade of Phabricator).

alex edited edge metadata.Sep 2 2016, 4:04 AM
alex added a project: Restricted Project.
alex requested changes to this revision.Nov 16 2016, 4:37 AM
alex edited edge metadata.

Please rebuild this patch based on In-Portal 5.2.x version.

This revision now requires changes to proceed.Nov 16 2016, 4:37 AM
erik added a comment.Dec 5 2016, 5:38 AM

Today looks that made all necessary changes, but not full-tested yet. Left part of tests planned on next day.

erik updated this revision to Diff 658.Dec 6 2016, 4:44 AM
erik edited edge metadata.

Patch rebuilt for 5.2.x version.

alex requested changes to this revision.Dec 6 2016, 6:31 AM
alex edited edge metadata.

Haven't tested code yet, but here are needed corrections.

core/kernel/application.php
736 ↗(On Diff #658)

remove this line

no real reason for this class to be placed in core/kernel folder

core/kernel/utility/template_comment_parser.php
1 ↗(On Diff #658)
  1. move this file to the core/kernel/units/helpers folder (no file/class rename is needed)
  2. register this class in core/kernel/units/helpers/helpers_config.php unit config file

no real reason for this class to be placed in core/kernel folder

2–15 ↗(On Diff #658)

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 ↗(On Diff #658)

remove this

there should be no description in file licensing comment

7 ↗(On Diff #658)

please update year to 2016

the end year in license should match year, when this file is comitted

54 ↗(On Diff #658)

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 ↗(On Diff #658)

move this to the constructor

we shouldn't be allowing to create parser from non-existing file in the first place

88–90 ↗(On Diff #658)

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 ↗(On Diff #658)
  1. change have (plural form) to has (singular form)
  2. inline variable
  3. correct grammar in each error message
  4. to compensate for long exception messages use sprintf for parameter injection, e.g.:
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 ↗(On Diff #658)

replace empty line + break with break + empty line

CS

122 ↗(On Diff #658)

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
127

inline this variable

variable isn't used only once

This revision now requires changes to proceed.Dec 6 2016, 6:31 AM
erik marked 12 inline comments as done.Apr 6 2017, 6:00 AM

today made all code changes, but not tested by plan yet

erik edited the test plan for this revision. (Show Details)Apr 10 2017, 5:41 AM
erik edited edge metadata.
erik edited the test plan for this revision. (Show Details)
erik updated this revision to Diff 721.Apr 10 2017, 6:07 AM
erik edited edge metadata.

Made all requested changes.

alex requested changes to this revision.Apr 12 2017, 6:03 AM
alex edited edge metadata.

Some of requested changes might need changing the test plan as well.

core/units/helpers/template_comment_parser.php
37–38

Please remove.

These 2 settings look as ones in your test plan.

77–82

Please transform this into block comment (not DocBlock):

/* 
 * Template starts with comment in such format:
 * <!--##
 * <NAME></NAME>
 * <DESC></DESC>
 * <SECTION>||</SECTION>
 * ##-->
*/
87

Please replace with:

throw new Exception(sprintf(
    'Unclosed meta-comment in "%s" template file.',
    $this->templateFile
));

Error message now is more user friendly.

100

Remove.

Doesn't really shed more light on error causes.

101

Please replace with:

throw new Exception(sprintf(
    'Meta-comment syntax error in "%s" template file.',
    $this->templateFile
));

Error message now is more user friendly.

109

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.

111

Replace with:

$setting_name,
'"' . implode('", "', array_keys($this->supportedSettings)) . '"'

Error message now is more user friendly.

116

Please:

  1. replace $result[$result_key] = trim($setting); with $result_value = trim($setting);
  2. in the switch below operate only on $result_value variable
  3. after end of the switch (before end of the cycle) do $result[$result_key] = $result_value;

Array key access for validation is removed (micro-optimization).

130

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.

136–142

Please combine this and above checks into same IF sharing same error message.

The error message was updated to reflect both cases.

148

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.

This revision now requires changes to proceed.Apr 12 2017, 6:03 AM
erik updated this revision to Diff 724.Apr 13 2017, 6:05 AM
erik edited edge metadata.
erik marked 10 inline comments as done.

Made all requested fixes.

erik edited the test plan for this revision. (Show Details)Apr 13 2017, 6:06 AM
erik edited edge metadata.
alex requested changes to this revision.May 22 2017, 4:49 AM
alex edited edge metadata.
alex added inline comments.
core/units/helpers/template_comment_parser.php
4
  1. replace $Id: image_helper.php 16247 2015-09-02 20:48:06Z alex $ with $Id
  2. add svn:keywords svn property with Id value

The $Id$ property isn't up to date.

18

Add final keyword to class declaration.

93
  1. After this line add code, that will transform & into &amp; without changing existing &amp; into &amp;amp;
  2. Update test plan to test also 4 cases:
    • no & or &amp; inside node value
    • only & inside node value
    • only &amp; inside node value
    • both &amp; and & inside node value

Solves BC break, that cause XML parsing error for meta comments using & instead of &amp;.

110
  1. Replace
$setting_name = $setting->getName();
$setting_value = trim($setting);
  1. Inline both variables when calling new parseSettingValue method.
112–153

Move out into parseSettingValue($name, $value) method, that will return $result_value variable.

121

This variable would only be needed after extracted method call.

122

When code around this place would be extracted into new method the initial value of this variable would come from method parameters.

134

Please replace $parsed < 1 with $parsed < 0.

The 0 is considered as positive number as well.

core/units/helpers/themes_helper.php
452–454
  1. Restore parseTemplateMetaInfo method, but it's body would be calling of new helper class.
  2. Add public scope and DocBlock to the method.

Avoids BC break when method is removed.

P.S.
Maybe before I've asked to remove parseTemplateMetaInfo method, sorry for that.

core/units/theme_files/theme_file_eh.php
122–128
  1. restore this code
  2. replace $file_description = array_key_exists('desc', $meta_info) ? $meta_info['desc'] : ''; with $file_description = $meta_info['desc'];
  3. inline $file_description variable

Prevents BC break (see previous comments).

This revision now requires changes to proceed.May 22 2017, 4:50 AM
alex added inline comments.May 24 2017, 3:09 AM
core/units/helpers/template_comment_parser.php
93

Actually don't do this.

99–109

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 &amp; and such) is now causing issues.

erik added a comment.Aug 7 2017, 6:03 AM

Today written locally all requested changes code, not tested, stored to Shelf.

erik edited the test plan for this revision. (Show Details)Aug 8 2017, 5:51 AM
erik updated this revision to Diff 813.Aug 8 2017, 5:57 AM
erik edited edge metadata.
erik edited the test plan for this revision. (Show Details)

Made all requested improvements.

Revision Contents
Changeset List

DiffIDBaseDescriptionCreatedLintUnit
BaseBase
Diff 152016347Apr 18 2016, 4:37 AM
★
★
Diff 265816502Patch rebuilt for 5.2.x version.Dec 6 2016, 4:44 AM
★
★
Diff 372116543Made all requested changes.Apr 10 2017, 6:07 AM
★
★
Diff 472416543Made all requested fixes.Apr 13 2017, 6:05 AM
★
★
Diff 581316600Made all requested improvements.Aug 8 2017, 5:57 AM
★
★

Diff 813

View Options

core/units/helpers/helpers_config.php

Loading...
View Options

core/units/helpers/template_comment_parser.php

Loading...
View Options

core/units/helpers/themes_helper.php

Loading...
View Options

core/units/theme_files/theme_file_eh.php

Loading...
In-Portal CMS · In-Portal Community Pages · In-Portal Issue Tracker · In-Portal Repository Browser · Quality Assurance · In-Portal CI Server