Page MenuHomeIn-Portal Phabricator
Differential D120

INP-1470 - Google Sitemap support
Needs ReviewPublic
Actions

Authored by erik on Sep 3 2015, 10:45 AM.

Details

Reviewers
alex
Summary

Feature to hide sitemap invisible sections/categories unless debug mode is turned on.

Test Plan

Part 1

  1. Go to Website & Content → Structure & Data section.
  2. Choose category, that has associated physical template (e.g. No Permissions).
  3. Delete that category.
  4. Go to Configuration → Website → Themes section.
  5. Press Rescan Themes button.
  6. Refresh page (F5).
  7. Confirm that deleted category was created automatically.

Part 2

  1. Go to Website & Content → Structure & Data section.
  2. Choose category, that has associated physical template (e.g. No Permissions).
  3. Delete that category.
  4. Put <IN_SITEMAP>always</IN_SITEMAP> in meta comment of associated template.
  5. Go to Configuration → Website → Themes section.
  6. Press Rescan Themes button.
  7. Confirm that exception about invalid in_sitemap value is shown.

Part 3

NOTE: Please repeat below steps with no, false and 0 values.
  1. Go to Website & Content → Structure & Data section.
  2. Choose category, that has associated physical template (e.g. No Permissions).
  3. Delete that category.
  4. Put <IN_SITEMAP>no</IN_SITEMAP> in meta comment of associated template.
  5. Go to Configuration → Website → Themes section.
  6. Press Rescan Themes button.
  7. Confirm, that deleted category was created automatically.
  8. Confirm, that created category has Include in Sitemap checkbox unchecked.

Part 4

NOTE: Please repeat below steps with yes, true and 1 values.
  1. Go to Website & Content → Structure & Data section.
  2. Choose category, that has associated physical template (e.g. No Permissions).
  3. Delete that category.
  4. Ensure <IN_SITEMAP>yes</IN_SITEMAP> in meta comment of associated template.
  5. Go to Configuration → Website → Themes section.
  6. Press Rescan Themes button.
  7. Confirm, that deleted category was created automatically.
  8. Confirm, that created category has Include in Sitemap checkbox unchecked.

Part 5

  1. Go to Website & Content → Structure & Data section.
  2. Choose category, that has associated physical template (e.g. No Permissions).
  3. Delete that category.
  4. Remove the <IN_SITEMAP>...</IN_SITEMAP> from meta comment of associated template.
  5. Go to Configuration → Website → Themes section.
  6. Press Rescan Themes button.
  7. Confirm, that deleted category was created automatically.
  8. Confirm, that created category has Include in Sitemap checkbox checked.

Part 6

  1. Go to Website & Content → Structure & Data section.
  2. Choose category, that has associated physical template (e.g. No Permissions).
  3. Delete that category.
  4. Put the <IN_SITEMAP/> in meta comment of associated template.
  5. Go to Configuration → Website → Themes section.
  6. Press Rescan Themes button.
  7. Confirm, that deleted category was created automatically.
  8. Confirm, that created category has Include in Sitemap checkbox checked.

Part 7

  1. Go to Configuration → Website → Output section.
  2. Confirm that Sections hidden from Sitemap system setting is present and has Always Shown in Catalog value.
  3. Go to Website & Content → Structure & Data section.
  4. Uncheck Include in Sitemap checkbox for any category.
  5. Confirm, that regardless of debug mode (enabled or disabled) that category is visible:
    • in catalog
    • in Parent Section dropdown during category editing
    • in internal pages dropdown, when adding link in CKEditor

Part 8

  1. Go to Configuration → Website → Output section.
  2. Change Sections hidden from Sitemap system setting value to Shown in Catalog Only in Debug Mode.
  3. Go to Website & Content → Structure & Data section.
  4. Confirm, that with debug mode enabled category with Show in Sitemap = No checkbox is visible:
    • in catalog
    • in Parent Section dropdown during category editing
    • in internal pages dropdown, when adding link in CKEditor
  5. Confirm, that with debug mode disabled that category with Show in Sitemap = No checkbox is hidden:
    • from catalog
    • from Parent Section dropdown during category editing
    • from internal pages dropdown, when adding link in CKEditor

Part 9

  1. Go to Website & Content → Structure & Data section.
  2. Edit some categories, to ensure both possible Include in Sitemap values - Yes and No - among different categories.
  3. Launch generate_sitemap scheduled task.
  4. Confirm, that is created /system/sitemaps/sitemap.xml.gz file.
  5. Confirm, that /system/sitemaps/sitemap.xml.gz file contains info about categories, included in sitemap, and does not contain info about others categories.

Part 10

  1. select simple theme
  2. change privacy.tpl of this theme - ensure <SECTION>Folder</SECTION> in the header part
  3. delete Privacy Policy category from catalog
  4. go to Configuration → Website → Themes section
  5. press Rescan Themes button
  6. confirm, that Folder category was created automatically an it has Include in Sitemap checkbox unchecked

Part 11 - testing structure cache rebuild

  1. Check/change config.php to enable Memcache.
  2. Go to Tools → System Tools → section.
  3. Press Reset button in the Memory Cache section.
  4. Confirm, that data exists under master:StructureTree cache.
  5. Comment out code line $data['cache_format'] = self::CACHE_FORMAT; in the CategoryHelper::_getStructureTree method.
  6. Add $data['rebuilt_on'] = TIMENOW; code line besides code line, commented before.
  7. Go to Website & Content1 → Structure & Data section.
  8. Go to Tools → System Tools section.
  9. Confirm that data exists under master:StructureTree cache.
  10. Confirm that data under master:StructureTree cache has 'rebuilt_on' key in the end.
  11. Confirm that value of 'rebuilt_on' key in the end of master:StructureTree cache differs from same value from previous cache rebuilt cycle.
  12. Add $data['cache_format'] = 1; code line besides code line, commented before.
  13. Go to Website & Content1 → Structure & Data section.
  14. Go to Tools → System Tools section.
  15. Confirm that data exists under master:StructureTree cache.
  16. Confirm that data under master:StructureTree cache has 'rebuilt_on' key among last 2 keys.
  17. Confirm that value of 'rebuilt_on' key in the end of master:StructureTree cache differs from same value from previous cache rebuilt cycle.
  18. Confirm that data under master:StructureTree cache has 'cache_format' key among last 2 keys.
  19. Confirm that value of 'cache_format' key in the end of master:StructureTree cache is 1.
  20. Go to Website & Content1 → Structure & Data section.
  21. Go to Tools → System Tools section.
  22. Confirm that data exists under master:StructureTree cache.
  23. Confirm that data under master:StructureTree cache has 'rebuilt_on' key among last 2 keys.
  24. Confirm that value of 'rebuilt_on' key in the end of master:StructureTree cache differs from same value from previous cache rebuilt cycle.
  25. Change recent $data['cache_format'] = 1; code line to $data['cache_format'] = 2;.
  26. Go to Website & Content1 → Structure & Data section.
  27. Go to Tools → System Tools section.
  28. Confirm that data exists under master:StructureTree cache.
  29. Confirm that data under master:StructureTree cache has 'rebuilt_on' key in the end.
  30. Confirm that value of 'rebuilt_on' key in the end of master:StructureTree cache differs from same value from previous cache rebuilt cycle.
  31. Confirm that value of 'cache_format' key in the end of master:StructureTree cache is 2.
  32. Go to Website & Content1 → Structure & Data section.
  33. Go to Tools → System Tools section.
  34. Confirm that data exists under master:StructureTree cache.
  35. Confirm that data under master:StructureTree cache has 'rebuilt_on' key in the end.
  36. Confirm that value of 'rebuilt_on' key in the end of master:StructureTree cache not changed and has same value as at previous check.

Diff Detail

Repository
rINP In-Portal
Branch
/in-portal/branches/5.3.x
Lint
Lint ErrorsExcuse: constants re-formatting is not a part of this task
SeverityLocationCodeMessage
Errorcore/kernel/constants.php:237PHPCS.E.CodingStandard.Classes.ClassDeclaration.SpaceBeforeKeywordCodingStandard.Classes.ClassDeclaration.SpaceBeforeKeyword
Errorcore/kernel/constants.php:238PHPCS.E.CodingStandard.Classes.ClassDeclaration.SpaceBeforeBraceCodingStandard.Classes.ClassDeclaration.SpaceBeforeBrace
Errorcore/kernel/constants.php:241PHPCS.E.CodingStandard.Classes.ClassDeclaration.SpaceBeforeCloseBraceCodingStandard.Classes.ClassDeclaration.SpaceBeforeCloseBrace
Unit
No Unit Test Coverage
Build Status
Buildable 203
Build 203: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
alex added inline comments.Oct 1 2015, 4:44 AM
core/install/upgrades.sql
3031–3034

Please use single database query, that would operate on system setting position rather then their names. It can look like this one used before:

UPDATE SystemSettings
SET DisplayOrder = ROUND(DisplayOrder - 0.01, 2)
WHERE (DisplayOrder BETWEEN 60.04 AND 60.10) AND (ModuleOwner = 'In-Portal') AND (Section = 'in-portal:configure_advanced');

Better yet increase upper bound for DisplayOrder to affect record with DisplayOrder up to, but not including 20.

System variables presence/order change change order time/project, but upgrade SQL needs to be clever enough to account for that.

3035

Please use same database query as in install_data.sql file.

Different database query used to do the same thing as on Clean Install.

core/units/helpers/category_helper.php
120

Please remove this.

  1. Performance penalty, because static check (doesn't use any of category data in check) is made for every category in the database.
  2. Check is already performed in CategoryHelper::getStructureTreeAsOptions method, that calls this method.
This revision now requires changes to proceed.Oct 1 2015, 4:44 AM
erik added inline comments.Oct 2 2015, 5:00 AM
core/units/helpers/category_helper.php
120

Not clear why we may be sure that this method is callable only from getStructureTreeAsOptions. Miserable performance penalty is better, than getting shown disallowed categories when somebody use this method in another context.

erik added inline comments.Oct 2 2015, 6:12 AM
core/units/helpers/category_helper.php
121–122

1.1. Not every category, but only categories that have children.
1.2. I do not know how you estimate performance - my tests show for old code and new code
new - 1.1920928955078E-6
old - 9.5367431640625E-7
old code is 50x+ times faster

this is not time consuming operation, and additional subcalls/checks consumes more performance, than probable economy on internal functionality.

alex added inline comments.Oct 2 2015, 6:44 AM
core/units/helpers/category_helper.php
120

We can't, but thanks to the fact, that method is protected (only class itself or it's descendants can call it) we don't care in this particular case.

  1. Addition of Admin Console Only check breaks BC (backwards compatibility) and makes _printChildren method work differently on Front-End.
  2. It's up to caller of the method to impose Admin Console Only restriction on method results.
  3. Task doesn't say directly or indirectly, that IncludeInSitemap functionality should only affect Admin Console.
121–122

1.1. Not every category, but only categories that have children.

This is related to what part of my original comment?

1.2. I do not know how you estimate performance - my tests show for old code and new code

Are you sure, that you've followed fixing plan from original comment (store result of new method in class property and use it here)?

erik updated this revision to Diff 361.Oct 5 2015, 11:26 AM
erik edited edge metadata.

Made requested fixes. Not tested In-Portal upgrade case.

alex added a comment.EditedOct 6 2015, 6:17 AM

Patch tested in current state (except upgrade). Please fix items below as well.

core/kernel/constants.php
237

Please name Visibility class into NonSitemapSectionVisibility.

Too general class name used, but constants inside class are very specific.

core/units/helpers/category_helper.php
34

Please add . at the end.

CS

42–43

Please remove.

The @return tag can't be used in __construct method (class constructor) command, because it technically doesn't have return statement inside it.

144–149

Please rewrite to avoid calling ConfigValue method (which does query database), when it's known upfront that value of that system setting won't be used.

Here is a helper table that just reflects current logic:

core/units/helpers/themes_helper.php
555–556

Please replace with:

$error_msg = 'The "IN_SITEMAP" setting in "' . $template_file . '" template ';
$error_msg .= 'has unsupported value. Please use "yes" or "no".';
throw new Exception($error_msg);

String operation right inside new class (exception) instantiation doesn't break any CS rules, but doesn't look in usual way.

alex requested changes to this revision.Oct 6 2015, 6:17 AM
alex edited edge metadata.
This revision now requires changes to proceed.Oct 6 2015, 6:17 AM
alex added a comment.Oct 6 2015, 1:19 PM

The main part that is missing in both test plan & implementation is actual Google Sitemap support, that is supposed to be there based on task name.

erik edited the test plan for this revision. (Show Details)Oct 8 2015, 4:10 AM
erik edited edge metadata.
erik updated this revision to Diff 371.Oct 8 2015, 4:39 AM
erik edited edge metadata.

Added Google maps support, made other requestes fixes.

alex requested changes to this revision.Oct 22 2015, 6:36 AM
alex edited edge metadata.

Please also add <IN_SITEMAP>false</IN_SITEMAP> to the meta comment of login-protected advanced theme templates (e.g. My Account and such).

core/install/upgrades.sql
3030

Please add following database query after this line:

UPDATE Categories SET IncludeInSitemap = 0 WHERE ThemeId <> 0 AND Type = 1;

Will exclude all categories, that were auto-created to store template files (have red folder icon in categories grid) from sitemap.

core/units/admin/admin_events_handler.php
1225 ↗(On Diff #371)

This is because of http://community.in-portal.org/pages/viewpage.action?pageId=16187694.

1231 ↗(On Diff #371)

This is because of http://community.in-portal.org/pages/viewpage.action?pageId=16187694.

core/units/categories/categories_event_handler.php
2049

Please add following above this line (and test that it works as expected):

$in_sitemap = false;

It's needed to avoid adding virtual pages, created during theme scan, to sitemap by default. There is also matching upgrade sql that does it.

core/units/helpers/helpers_config.php
69 ↗(On Diff #371)

Please remove this.

In In-Portal 5.3.x class/interface/trait location is determined automatically thanks to class_structure.php file in each module (including Core).

core/units/helpers/sitemap_helper.php
110 ↗(On Diff #371)

Please remove /user_files part.

The generated sitemaps are not user uploaded content.

182–186 ↗(On Diff #371)
  1. remove this
  2. iterate over all enabled (skip Core and In-Portal) modules and call the $this->addCategoryItems method with their main prefix

Items from other modules not added.

229–269 ↗(On Diff #371)

Please also apply changes from rEBN16606, which was mentioned in JIRA task.

Not all content from commits mentioned in JIRA task were applied.

282 ↗(On Diff #371)

When unit supplied via $prefix variable doesn't have IDFIeld or TableName don't do nothing.

This revision now requires changes to proceed.Oct 22 2015, 6:36 AM
erik edited the test plan for this revision. (Show Details)Oct 26 2015, 5:33 AM
erik edited edge metadata.
erik updated this revision to Diff 394.Oct 26 2015, 6:01 AM
erik edited edge metadata.
erik edited the test plan for this revision. (Show Details)

Applued additional patch, made other requested changes.

alex requested changes to this revision.Dec 7 2015, 5:04 AM
alex edited edge metadata.
alex added inline comments.
core/units/helpers/sitemap_helper.php
183 ↗(On Diff #394)

Please remove && $module_info['Loaded'] part.

The $this->Application->ModuleInfo array contains information about loaded modules only.

This revision now requires changes to proceed.Dec 7 2015, 5:04 AM
alex added inline comments.Dec 7 2015, 5:13 AM
core/install/cache/class_structure.php
170 ↗(On Diff #394)

Please run in-portal classmap:rebuild command.

The file contents is out of date.

alex added a comment.Dec 7 2015, 6:01 AM

I see following warnings when full Advanced theme rescan is happening:

  • simplexml_load_string(): Entity: line 1: parser error : xmlParseEntityRef: no name
  • simplexml_load_string(): <meta_info>CSS & JAVASCRIPT FOR IN-COMMERCE</meta_info>
  • simplexml_load_string(): ^

This seems to be happening because /in-commerce/elements/html_head.elm.tpl template has following on top of it:

<!--## CSS & JAVASCRIPT FOR IN-COMMERCE ##-->

which does look like a template HTML and is parsed as such. Probably replacing & with &amp; should do it.

alex added a comment.Dec 7 2015, 6:44 AM

Please revert change, that makes red categories not present in sitemap by default (seems to be 2 places: sql during upgrade and $in_sitemap = false during auto-creation), because it creates nasty side effect: when debug mode is enabled I don't see Platform category (IncludeInSitemap=0), while it's sub-categories (e.g. Maintenance Mode) are marked as IncludeInSitemap.

P.S.
I know I requested this change in first place, but you should have reported that side effect.

alex added inline comments.Dec 7 2015, 9:34 AM
core/units/helpers/sitemap_helper.php
236 ↗(On Diff #394)

Please remove this.

I have no idea, what top category means in here, because we're selecting all categories anyway.

247 ↗(On Diff #394)

Please remove this.

I have no idea, what top category means in here, because we're selecting all categories anyway.

248 ↗(On Diff #394)

Please inline $category_id variable.

After method extraction requested below this would be the only usage of that variable.

249–260 ↗(On Diff #394)

Please extract this to the addCategory($category_id) method.

alex added inline comments.Dec 7 2015, 9:55 AM
core/units/helpers/sitemap_helper.php
180–186 ↗(On Diff #394)

Please extract this method part into doBuild method.

Simplifies extension of this class for adding more info to existing sitemap.

erik updated this revision to Diff 431.Dec 8 2015, 4:55 AM
erik edited edge metadata.

Requested fixes and improvements.

alex edited edge metadata.Mar 10 2016, 5:08 AM
alex added a project: Restricted Project.
alex requested changes to this revision.Mar 30 2016, 3:49 AM
alex edited edge metadata.

These are mostly optimization related corrections.

core/units/helpers/sitemap_helper.php
246–247 ↗(On Diff #431)

Please replace with:

/** @var kDBList $category_list */
$category_list = $this->Application->recallObject('c.sitemap', 'c_List', $tag_params);

CS

250 ↗(On Diff #431)

Please change this into:

$category_list->ClearOrderFields();
$category_list->AddOrderField('ParentPath', 'asc');

The setOrderFields is method added in In-Portal 5.2.x only and therefore attempt to reuse this class on In-Portal 5.1.x based projects will fail.

251–255 ↗(On Diff #431)

Please change this into:

$sql = $category_list->getCountSQL($category_list->GetSelectSQL(true));
$sql = str_replace(
    'COUNT(*) AS count',
    $category_list->TableName . '.' . $category_list->IDField,
    $sql
);

$categories = $this->Conn->GetCol($sql);

foreach ( $categories as $category_id ) {
    $this->addCategory($category_id);
}

Above proposed solution only selects needed fields.

482–496 ↗(On Diff #431)

Please do this:

  1. extract this as getDefaultUrlParams method
  2. create defaultUrlParams class property
  3. in the class constructor (after calling parent class constructor) call above method
  4. then this whole method becomes return array_merge($this->defaultUrlParams, $custom_params);

This is an optimization because we're doing 2 IFs here which yield same result no matter how much times we call them.

This revision now requires changes to proceed.Mar 30 2016, 3:49 AM
erik edited the test plan for this revision. (Show Details)Mar 30 2016, 10:05 AM
erik edited edge metadata.
alex edited the test plan for this revision. (Show Details)Mar 30 2016, 10:40 AM
erik updated this revision to Diff 513.Mar 30 2016, 11:12 AM
erik edited edge metadata.

Made requested optimization changes.

alex requested changes to this revision.Mar 30 2016, 11:18 AM
alex edited edge metadata.
alex added inline comments.
core/units/helpers/sitemap_helper.php
51 ↗(On Diff #513)

Please change string to array.

The array is stored in there, not string.

53 ↗(On Diff #513)

Please assign an empty array here.

Change needed to stress property data type.

260 ↗(On Diff #513)

Please add an empty line above this line.

To visually separate sorting change from SQL building.

This revision now requires changes to proceed.Mar 30 2016, 11:18 AM
erik updated this revision to Diff 514.Apr 1 2016, 4:28 AM
erik edited edge metadata.

Fixed class variable type, made requested code style fixes.

alex requested changes to this revision.Nov 15 2016, 6:44 AM
alex edited edge metadata.

Please add a code into Upgrade_5_3_0_B1 method in core/install/upgrades.php file, that when $mode == 'after' will call c:OnResetCMSMenuCache event to reset SMS menu cache, that doesn't have IncludeInSitemap key.

This revision now requires changes to proceed.Nov 15 2016, 6:44 AM
alex added a comment.Nov 16 2016, 4:16 AM
In D120#4890, @alex wrote:

Please add a code into Upgrade_5_3_0_B1 method in core/install/upgrades.php file, that when $mode == 'after' will call c:OnResetCMSMenuCache event to reset SMS menu cache, that doesn't have IncludeInSitemap key.

Actually instead of this let's make cache self-aware:

  1. in CategoryHelper class add const CACHE_FORMAT = 2; before any other class property is declared
  2. in CategoryHelper::_getStructureTree method:
    • when we've got non-empty data from cache look at $data['cache_format'] key
    • if key is absent OR value is smaller, then self::CACHE_FORMAT (above added constant) then don't return obtained cache and proceed to code that rebuilds it
    • right after cache is done building (the outer _getChildren call), but before it's stored to memcache/db add cache version it like so: $data['cache_format'] = self::CACHE_FORMAT;

Update test plan to reflect these scenarios:

  • there is no structure cache = cache rebuild happens
  • there is structure cache without cache_format in it = cache rebuild happens
  • there is is structure cache with smaller cache_format than needed = cache rebuild happens
  • there is is structure cache with correct cache_format = no cache rebuild happens
erik added a comment.Dec 6 2016, 5:07 AM

Today only re-applied differential patch and re-installed local version of 5.3.x In-Portal with this patch.

erik added a comment.Dec 7 2016, 5:02 AM

Today made all necessary changes locally, started writing additions to the test plan.

erik edited the test plan for this revision. (Show Details)Jul 25 2017, 6:18 AM
erik updated this revision to Diff 806.Jul 26 2017, 6:06 AM
erik edited edge metadata.
erik edited the test plan for this revision. (Show Details)

Made requested changes to category helper.

Revision Contents
Changeset List

DiffIDBaseDescriptionCreatedLintUnit
BaseBase
Diff 129616249Sep 3 2015, 10:45 AM
★
★
Diff 236116284Made requested fixes. Not tested In-Portal upgrade case.Oct 5 2015, 11:26 AM
★
★
Diff 337116285Added Google maps support, made other requestes fixes.Oct 8 2015, 4:39 AM
★
★
Diff 439416300Applued additional patch, made other requested changes.Oct 26 2015, 6:01 AM
★
★
Diff 543116303Requested fixes and improvements.Dec 8 2015, 4:55 AM
★
★
Diff 651316334Made requested optimization changes.Mar 30 2016, 11:12 AM
★
★
Diff 751416337Fixed class variable type, made requested code style fixes.Apr 1 2016, 4:28 AM
★
★
Diff 880616595Made requested changes to category helper.Jul 26 2017, 6:06 AM
★
★

Diff 361

View Options

core/admin_templates/categories/categories_edit.tpl

Loading...
View Options

core/install/english.lang

Loading...
View Options

core/install/install_data.sql

Loading...
View Options

core/install/install_schema.sql

Loading...
View Options

core/install/upgrades.sql

Loading...
View Options

core/kernel/constants.php

Loading...
View Options

core/units/categories/categories_config.php

Loading...
View Options

core/units/categories/categories_event_handler.php

Loading...
View Options

core/units/helpers/category_helper.php

Loading...
View Options

core/units/helpers/themes_helper.php

Loading...
View Options

core/units/structure/structure_config.php

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