Feature to hide sitemap invisible sections/categories unless debug mode is turned on.
Details
- Reviewers
alex
Part 1
- Go to Website & Content → Structure & Data section.
- Choose category, that has associated physical template (e.g. No Permissions).
- Delete that category.
- Go to Configuration → Website → Themes section.
- Press Rescan Themes button.
- Refresh page (F5).
- Confirm that deleted category was created automatically.
Part 2
- Go to Website & Content → Structure & Data section.
- Choose category, that has associated physical template (e.g. No Permissions).
- Delete that category.
- Put <IN_SITEMAP>always</IN_SITEMAP> in meta comment of associated template.
- Go to Configuration → Website → Themes section.
- Press Rescan Themes button.
- Confirm that exception about invalid in_sitemap value is shown.
Part 3
- Go to Website & Content → Structure & Data section.
- Choose category, that has associated physical template (e.g. No Permissions).
- Delete that category.
- Put <IN_SITEMAP>no</IN_SITEMAP> in meta comment of associated template.
- Go to Configuration → Website → Themes section.
- Press Rescan Themes button.
- Confirm, that deleted category was created automatically.
- Confirm, that created category has Include in Sitemap checkbox unchecked.
Part 4
- Go to Website & Content → Structure & Data section.
- Choose category, that has associated physical template (e.g. No Permissions).
- Delete that category.
- Ensure <IN_SITEMAP>yes</IN_SITEMAP> in meta comment of associated template.
- Go to Configuration → Website → Themes section.
- Press Rescan Themes button.
- Confirm, that deleted category was created automatically.
- Confirm, that created category has Include in Sitemap checkbox unchecked.
Part 5
- Go to Website & Content → Structure & Data section.
- Choose category, that has associated physical template (e.g. No Permissions).
- Delete that category.
- Remove the <IN_SITEMAP>...</IN_SITEMAP> from meta comment of associated template.
- Go to Configuration → Website → Themes section.
- Press Rescan Themes button.
- Confirm, that deleted category was created automatically.
- Confirm, that created category has Include in Sitemap checkbox checked.
Part 6
- Go to Website & Content → Structure & Data section.
- Choose category, that has associated physical template (e.g. No Permissions).
- Delete that category.
- Put the <IN_SITEMAP/> in meta comment of associated template.
- Go to Configuration → Website → Themes section.
- Press Rescan Themes button.
- Confirm, that deleted category was created automatically.
- Confirm, that created category has Include in Sitemap checkbox checked.
Part 7
- Go to Configuration → Website → Output section.
- Confirm that Sections hidden from Sitemap system setting is present and has Always Shown in Catalog value.
- Go to Website & Content → Structure & Data section.
- Uncheck Include in Sitemap checkbox for any category.
- 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
- Go to Configuration → Website → Output section.
- Change Sections hidden from Sitemap system setting value to Shown in Catalog Only in Debug Mode.
- Go to Website & Content → Structure & Data section.
- 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
- 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
- Go to Website & Content → Structure & Data section.
- Edit some categories, to ensure both possible Include in Sitemap values - Yes and No - among different categories.
- Launch generate_sitemap scheduled task.
- Confirm, that is created /system/sitemaps/sitemap.xml.gz file.
- 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
- select simple theme
- change privacy.tpl of this theme - ensure <SECTION>Folder</SECTION> in the header part
- delete Privacy Policy category from catalog
- go to Configuration → Website → Themes section
- press Rescan Themes button
- confirm, that Folder category was created automatically an it has Include in Sitemap checkbox unchecked
Part 11 - testing structure cache rebuild
- Check/change config.php to enable Memcache.
- Go to Tools → System Tools → section.
- Press Reset button in the Memory Cache section.
- Confirm, that data exists under master:StructureTree cache.
- Comment out code line $data['cache_format'] = self::CACHE_FORMAT; in the CategoryHelper::_getStructureTree method.
- Add $data['rebuilt_on'] = TIMENOW; code line besides code line, commented before.
- Go to Website & Content1 → Structure & Data section.
- Go to Tools → System Tools section.
- Confirm that data exists under master:StructureTree cache.
- Confirm that data under master:StructureTree cache has 'rebuilt_on' key in the end.
- Confirm that value of 'rebuilt_on' key in the end of master:StructureTree cache differs from same value from previous cache rebuilt cycle.
- Add $data['cache_format'] = 1; code line besides code line, commented before.
- Go to Website & Content1 → Structure & Data section.
- Go to Tools → System Tools section.
- Confirm that data exists under master:StructureTree cache.
- Confirm that data under master:StructureTree cache has 'rebuilt_on' key among last 2 keys.
- Confirm that value of 'rebuilt_on' key in the end of master:StructureTree cache differs from same value from previous cache rebuilt cycle.
- Confirm that data under master:StructureTree cache has 'cache_format' key among last 2 keys.
- Confirm that value of 'cache_format' key in the end of master:StructureTree cache is 1.
- Go to Website & Content1 → Structure & Data section.
- Go to Tools → System Tools section.
- Confirm that data exists under master:StructureTree cache.
- Confirm that data under master:StructureTree cache has 'rebuilt_on' key among last 2 keys.
- Confirm that value of 'rebuilt_on' key in the end of master:StructureTree cache differs from same value from previous cache rebuilt cycle.
- Change recent $data['cache_format'] = 1; code line to $data['cache_format'] = 2;.
- Go to Website & Content1 → Structure & Data section.
- Go to Tools → System Tools section.
- Confirm that data exists under master:StructureTree cache.
- Confirm that data under master:StructureTree cache has 'rebuilt_on' key in the end.
- Confirm that value of 'rebuilt_on' key in the end of master:StructureTree cache differs from same value from previous cache rebuilt cycle.
- Confirm that value of 'cache_format' key in the end of master:StructureTree cache is 2.
- Go to Website & Content1 → Structure & Data section.
- Go to Tools → System Tools section.
- Confirm that data exists under master:StructureTree cache.
- Confirm that data under master:StructureTree cache has 'rebuilt_on' key in the end.
- 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 Errors Excuse: file reformatting is not part of this task Severity Location Code Message Error core/kernel/constants.php:237 PHPCS.E.CodingStandard.Classes.ClassDeclaration.SpaceBeforeKeyword CodingStandard.Classes.ClassDeclaration.SpaceBeforeKeyword Error core/kernel/constants.php:238 PHPCS.E.CodingStandard.Classes.ClassDeclaration.SpaceBeforeBrace CodingStandard.Classes.ClassDeclaration.SpaceBeforeBrace Error core/kernel/constants.php:241 PHPCS.E.CodingStandard.Classes.ClassDeclaration.SpaceBeforeCloseBrace CodingStandard.Classes.ClassDeclaration.SpaceBeforeCloseBrace Error core/units/admin/admin_events_handler.php:1225 PHPCS.E.CodingStandard.Commenting.FunctionComment.NonEventShortNotCapital CodingStandard.Commenting.FunctionComment.NonEventShortNotCapital Error core/units/admin/admin_events_handler.php:1231 PHPCS.E.CodingStandard.NamingConventions.ValidFunctionName.ScopeNotCamelCaps CodingStandard.NamingConventions.ValidFunctionName.ScopeNotCamelCaps Error core/units/helpers/category_helper.php:38 PHPCS.E.Generic.WhiteSpace.ScopeIndent.IncorrectExact Generic.WhiteSpace.ScopeIndent.IncorrectExact Error core/units/helpers/category_helper.php:43 PHPCS.E.Generic.WhiteSpace.ScopeIndent.IncorrectExact Generic.WhiteSpace.ScopeIndent.IncorrectExact - Unit
No Unit Test Coverage - Build Status
Buildable 236 Build 236: arc lint + arc unit
Event Timeline
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 | ||
118 | Please remove this.
|
core/units/helpers/category_helper.php | ||
---|---|---|
118 | 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. |
core/units/helpers/category_helper.php | ||
---|---|---|
119–120 | 1.1. Not every category, but only categories that have children. this is not time consuming operation, and additional subcalls/checks consumes more performance, than probable economy on internal functionality. |
core/units/helpers/category_helper.php | ||
---|---|---|
118 | 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.
| |
119–120 |
This is related to what part of my original comment?
Are you sure, that you've followed fixing plan from original comment (store result of new method in class property and use it here)? |
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. | |
142–147 | 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. |
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.
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 | This is because of http://community.in-portal.org/pages/viewpage.action?pageId=16187694. | |
1231 | This is because of http://community.in-portal.org/pages/viewpage.action?pageId=16187694. | |
core/units/categories/categories_event_handler.php | ||
2050 | 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 | ||
111 | Please remove /user_files part. The generated sitemaps are not user uploaded content. | |
183–187 |
Items from other modules not added. | |
230–270 | Please also apply changes from rEBN16606, which was mentioned in JIRA task. Not all content from commits mentioned in JIRA task were applied. | |
283 | When unit supplied via $prefix variable doesn't have IDFIeld or TableName don't do nothing. |
core/units/helpers/sitemap_helper.php | ||
---|---|---|
183 | Please remove && $module_info['Loaded'] part. The $this->Application->ModuleInfo array contains information about loaded modules only. |
core/install/cache/class_structure.php | ||
---|---|---|
170 | Please run in-portal classmap:rebuild command. The file contents is out of date. |
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 & should do it.
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.
core/units/helpers/sitemap_helper.php | ||
---|---|---|
236 | Please remove this. I have no idea, what top category means in here, because we're selecting all categories anyway. | |
247 | Please remove this. I have no idea, what top category means in here, because we're selecting all categories anyway. | |
248 | Please inline $category_id variable. After method extraction requested below this would be the only usage of that variable. | |
249–260 | Please extract this to the addCategory($category_id) method. |
core/units/helpers/sitemap_helper.php | ||
---|---|---|
180–186 | Please extract this method part into doBuild method. Simplifies extension of this class for adding more info to existing sitemap. |
These are mostly optimization related corrections.
core/units/helpers/sitemap_helper.php | ||
---|---|---|
247–248 | Please replace with: /** @var kDBList $category_list */ $category_list = $this->Application->recallObject('c.sitemap', 'c_List', $tag_params); CS | |
251 | 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. | |
252–256 | 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. | |
483–497 | Please do this:
This is an optimization because we're doing 2 IFs here which yield same result no matter how much times we call them. |
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:
- in CategoryHelper class add const CACHE_FORMAT = 2; before any other class property is declared
- 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
Today only re-applied differential patch and re-installed local version of 5.3.x In-Portal with this patch.
Today made all necessary changes locally, started writing additions to the test plan.