Language Change PR-staging

Pending

User tests: Successful: Unsuccessful:

avatar SharkyKZ
SharkyKZ
27 Apr 2018

Pull Request for Issues #17270, #18458, #19661.

Note: this can be merged after #20626 is merged.

Summary of Changes

This PR introduces several features and fixes for Articles - Category module.

New Features:

  • Option to show article tags (#17270)
  • Option to group by tags (#18458)
  • Grouping by specific date field (#19661)
  • Article list moved to a sublayout to reduce code duplication

Fixes:

  • Fixed fatal PHP error when article has alternative read more
  • When grouping by author, articles without author now appear under "No Author" group
  • Hits are shown when articles has 0 hits
  • Tags are no longer loaded when filtering by tags
  • XML: added showon and filter attributes where appropriate
  • Changed author field type from SQL to author
  • Using correct default param values
  • Some attempts at CS and cleanup

Testing Instructions

Code review.

Publish Articles - Category module. Existing features work like before or like stated above.

Try new options:

  1. Enable Show Tags option.
  2. Set Article Grouping to Tags.
  3. Set Article Grouping to Year or Month Year and try different values for Date Grouping Field.

Expected result

  1. Article tags are shown.
  2. Articles are grouped by tags.
  3. Articles are grouped by selected date field.

Documentation Changes Required

New module options added.
New module helper class function added.
Updated existing class function doc blocks.

avatar SharkyKZ SharkyKZ - open - 27 Apr 2018
avatar SharkyKZ SharkyKZ - change - 27 Apr 2018
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 27 Apr 2018
Category Administration com_modules Language & Strings Modules Front End
0f6356b 27 Apr 2018 avatar SharkyKZ CS
avatar SharkyKZ SharkyKZ - change - 27 Apr 2018
Labels Added: Language Change PR-staging
avatar brianteeman
brianteeman - comment - 28 Apr 2018

Is this backwards compatible?

A PR really should address one issue only. When you fix things and add new features at the same time it makes it hard to test etc

avatar SharkyKZ SharkyKZ - change - 28 Apr 2018
The description was changed
avatar SharkyKZ SharkyKZ - edited - 28 Apr 2018
avatar SharkyKZ SharkyKZ - change - 28 Apr 2018
The description was changed
avatar SharkyKZ SharkyKZ - edited - 28 Apr 2018
avatar SharkyKZ SharkyKZ - change - 28 Apr 2018
The description was changed
avatar SharkyKZ SharkyKZ - edited - 28 Apr 2018
avatar SharkyKZ
SharkyKZ - comment - 28 Apr 2018

I'd like to think so.

New options are B/C safe because they need to be enabled to take effect.

Item data is largely unaffected, except for corrected tag loading behavior and grouping by author.

I did remove $item_heading variable which refers to param that was removed 5 years ago (#1131). For users who use template overrides from 5 years ago this could show undefined variable warning. If this is an issue, this can be reverted and held until 4.0.

avatar brianteeman
brianteeman - comment - 28 Apr 2018

I did remove $item_heading variable which refers to param that was removed 5 years ago (#1131). For uqsers who use template overrides from 5 years ago this could show undefined variable warning. If this is an issue, this can be reverted and held until 4.0.

For me that would be a b/c break

avatar SharkyKZ
SharkyKZ - comment - 28 Apr 2018

$item_heading restored.

avatar ReLater
ReLater - comment - 28 Apr 2018

I have tested this item 🔴 unsuccessfully on df0da20

See also images on GitHub.

"All Categories" hint is missing after applying patch. I think "All Categories" is a welcome hint for users.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20248.

avatar ReLater ReLater - test_item - 28 Apr 2018 - Tested unsuccessfully
avatar ReLater
ReLater - comment - 28 Apr 2018

Before patch

28-04-_2018_16-15-02

After patch

28-04-_2018_16-15-53

avatar infograf768
infograf768 - comment - 29 Apr 2018

@ReLater

"All Categories" hint is missing after applying patch. I think "All Categories" is a welcome hint for users.

That would mean that we change the default placeholder each time we use a multiple class , i.e. All Tags, All Categories, All Authors, All Access Levels, instead of Select ...

I am not in favor of that change.
We could evidently add for each of these fields' description/tip all over Joomla something like If you do not select anything, then etc. 17 times in present core, this PR included.
I am sure some would oppose strongly to that.

here are the files concerned

~/administrator/components/com_content/models/forms/filter_articles.xml:28: 			class="multipleCategories"
~/administrator/components/com_content/models/forms/filter_articles.xml:40: 			class="multipleAccessLevels"
~/administrator/components/com_content/models/forms/filter_articles.xml:50: 			class="multipleAuthors"
~/administrator/components/com_content/models/forms/filter_articles.xml:73: 			class="multipleTags"
~/administrator/components/com_content/models/forms/filter_featured.xml:28: 			class="multipleCategories"
~/administrator/components/com_content/models/forms/filter_featured.xml:53: 			class="multipleAccessLevels"
~/administrator/components/com_content/models/forms/filter_featured.xml:63: 			class="multipleAuthors"
~/administrator/components/com_content/models/forms/filter_featured.xml:86: 			class="multipleTags"
~/components/com_content/models/forms/filter_articles.xml:28: 			class="multipleCategories"
~/components/com_content/models/forms/filter_articles.xml:40: 			class="multipleAccessLevels"
~/components/com_content/models/forms/filter_articles.xml:50: 			class="multipleAuthors"
~/components/com_content/models/forms/filter_articles.xml:87: 			class="multipleTags"
~/modules/mod_articles_category/mod_articles_category.xml:106: 					class="multipleCategories"
~/modules/mod_articles_category/mod_articles_category.xml:147: 					class="multipleTags"
~/modules/mod_articles_category/mod_articles_category.xml:176: 					class="multipleAuthors"
~/modules/mod_articles_news/mod_articles_news.xml:32: 					class="multipleCategories"
~/modules/mod_articles_news/mod_articles_news.xml:44: 					class="multipleTags"
avatar infograf768
infograf768 - comment - 29 Apr 2018

@SharkyKZ
BTW, the default placeholder for Author Aliases is wrong as it proposes to type or select option.

I propose to add a class in the field
class="multipleAliases"
and evidently in module edit:
JHtml::_('formbehavior.chosen', '.multipleAliases', null, array('placeholder_text_multiple' => JText::_('JOPTION_SELECT_ALIAS')));

and add the new string in the en-GB.ini files (back and front)

5a2173a 29 Apr 2018 avatar SharkyKZ CS
be19007 29 Apr 2018 avatar SharkyKZ CS
avatar ReLater
ReLater - comment - 29 Apr 2018

That would mean that we change the default placeholder each time we use a multiple class , i.e. All Tags, All Categories, All Authors, All Access Levels, instead of Select ...

Would be at least helpful whenever we're talking about configuration options. A clear statement instead of may be or may be not "All Thangs"

I am not in favor of that change.

In this case the PR changed that. From my point of view not a bug...

avatar brianteeman
brianteeman - comment - 29 Apr 2018

this is why you never try to solve multiple issues in a single pr. it makes it impossible to test

avatar SharkyKZ
SharkyKZ - comment - 29 Apr 2018

@ReLater using empty string as default value is not OK. It causes incorrect values to be saved. This can be solved in the code (see #20245 for example) but incorrect values shouldn't be passed in the first place. And using filters on such fields causes more issues.

Then there's UX issue. Current implementation is misleading, if not completely wrong. Users can leave default option selected and select additional options. If you have "All Categories" and "Some Category" selected, logically the results should include items from all categories but instead include items from "Some Category". All in all, this wasn't very well thought out.

But those are just my personal comments. See #17668. This is the right way going forward.

avatar SharkyKZ
SharkyKZ - comment - 30 Apr 2018

@infograf768 This was left out because alias author is a custom SQL field at the moment. It needs to be added as core field but that's for another time.

avatar infograf768
infograf768 - comment - 30 Apr 2018

This was left out because alias author is a custom SQL field at the moment. It needs to be added as core field but that's for another time.

No need to be added as core field to modify the way we get the placeholder the way I suggested imho. What we have now as default placeholder is just plain wrong.

avatar SharkyKZ
SharkyKZ - comment - 30 Apr 2018

For UI consistency within this module, yes, the placeholder should be added. But that's the thing, at the moment this placeholder is needed specifically for this module, because author alias is not a SQL field.
Author alias does need to be added as core field because it needs to be reusable and because core shouldn't use SQL fields anyways. If you search for type="sql" you'll see author alias field in mod_articles_category.xml is the last remaining SQL field.

Anyways, I'm going to take your advice. But with idea of adding author alias field type later on.

To be honest, though, I'm not a fan of this placeholder approach in general. #12447 was the best solution yet, IMO. I'd like to see it revived.

avatar infograf768
infograf768 - comment - 1 May 2018

@SharkyKZ
Remains a question: shall we keep or not using the plural forms as the field is multiple?
We do have both variants in en-GB.ini. The plural form has only been used until now in core for this module.

JOPTION_SELECT_AUTHOR_ALIAS="- Select Author Alias -"
JOPTION_SELECT_AUTHOR_ALIASES="- Select Author Aliases -"
JOPTION_SELECT_AUTHOR="- Select Author -"
JOPTION_SELECT_AUTHORS="- Select Authors -"

The singular ones are commonly used when filtering in the managers.

avatar brianteeman
brianteeman - comment - 1 May 2018

The singular ones are commonly used when filtering in the managers.

Consistency wins so i would go with the singular. I would only use the plural in a place where it has to be a multiple

avatar Quy
Quy - comment - 11 May 2018

Problem loading page with error The connection was reset in Firefox with nightly build.

  • Install Test English (GB) Sample Data
  • Install patch
  • Under Extensions > Modules, edit Articles Category
  • Set to display on all pages in position-7 (right)
  • Under Module, select Dynamic
  • Under Display Options, show Introtext
  • Save
  • Go to frontend and click Similar Tags
avatar infograf768
infograf768 - comment - 11 May 2018

@Quy
I do not get any error with staging

avatar SharkyKZ
SharkyKZ - comment - 11 May 2018

@Quy do you install sample data during Joomla installation? Because I don't see Test English (GB) Sample Data option. Only these:

None (Required for basic native multilingual site creation)	
Blog English (GB) Sample Data	
Brochure English (GB) Sample Data	
Default English (GB) Sample Data	
Learn Joomla English (GB) Sample Data	

Tried with 3.8.8-dev and 3.9.0-dev.

avatar Quy
Quy - comment - 11 May 2018

@SharkyKZ Yes during Joomla installation with staging using XAMPP:
https://github.com/joomla/joomla-cms/archive/staging.zip

joomla-demo

When debug is on, I get this.

Fatal error: Allowed memory size of 134217728 bytes exhausted (tried to allocate 4096 bytes) in \libraries\src\Log\LogEntry.php on line 116

Fatal error: Allowed memory size of 134217728 bytes exhausted (tried to allocate 20480 bytes) in \libraries\vendor\joomla\registry\src\Registry.php on line 85

I will test it on a live server to see if it can be reproduced.

avatar Quy
Quy - comment - 11 May 2018

Nevermind. The issue occurs even without this PR.

avatar infograf768
infograf768 - comment - 11 May 2018

@SharkyKZ
We get test sample data by installing this:
https://github.com/joomla-extensions/testing-sample-data

avatar SharkyKZ
SharkyKZ - comment - 11 May 2018

@infograf768 thanks for the link.

@Quy you're getting an infinite loop.

Articles Category Module article contains Articles Category module (using Load Module plugin).
Articles Category module displays the article.
When introtext is enabled in the module, the module then displays the article text which contains the module.
So the module tries to render itself causing an infinite loop.

The issue isn't caused by this patch but in your specific case, the fixes in the patch allows the article containing the module to appear in the module. Before the patch, the articles would have been incorrectly filtered out.

avatar Quy
Quy - comment - 12 May 2018

For tags, it is missing itemscope itemtype="https://schema.org/Article" properties. Are they necessary?

<div class="mod-articles-category-tags">
                <ul class="tags inline">
                <li class="tag-2 tag-list0" itemprop="keywords">
                <a href="/joomla-cms-staging/tagged-items" class="label label-info">
                        Red					</a>
        </li>
                <li class="tag-3 tag-list1" itemprop="keywords">
                <a href="/joomla-cms-staging/compact-tagged" class="label label-info">
                        Yellow					</a>
        </li>
                <li class="tag-4 tag-list2" itemprop="keywords">
                <a href="/joomla-cms-staging/all-tags/green" class="label label-info">
                        Green					</a>
        </li>
                        </ul>
</div>
avatar SharkyKZ
SharkyKZ - comment - 15 May 2018

What are Joomla's standards for microdata? Should Google's Sructured Data Testing Tool be used for validation? If so, adding Article itemtype would only result in multiple validation errors due to missing data (the same is true for article views and other modules). Meanwhile, loose itemprop attributes show no errors.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 16 Jun 2018

@SharkyKZ can you please create new PRs adressing one Issue at once as @brianteeman wrote above?

Its easier to test one by one.

avatar SharkyKZ SharkyKZ - change - 18 Jun 2018
The description was changed
avatar SharkyKZ SharkyKZ - edited - 18 Jun 2018
avatar SharkyKZ SharkyKZ - change - 18 Jun 2018
The description was changed
avatar SharkyKZ SharkyKZ - edited - 18 Jun 2018
avatar SharkyKZ
SharkyKZ - comment - 18 Jun 2018

@franz-wohlkoenig general fixes should be covered by code review and new features are simple to test as they're just new grouping/display options. But I have updated testing instructions to indicate these new options.

avatar brianteeman
brianteeman - comment - 18 Jun 2018

I am not in favour of this change #20248 (comment)

avatar SharkyKZ
SharkyKZ - comment - 18 Jun 2018

@brianteeman placeholder with empty value is a leftover from single selects. It will need to be removed from all multi selects. See #20248 (comment) for more details.

avatar brianteeman
brianteeman - comment - 18 Jun 2018

Because of the addition of showon="date_filtering:range" (which is great) the following strings need updating and simplifying. We don't need the "If xxx is selected above"


MOD_ARTICLES_CATEGORY_FIELD_ENDDATE_DESC="If Date Range is selected above, please enter an End Date."
MOD_ARTICLES_CATEGORY_FIELD_RELATIVEDATE_DESC="If Relative Date is selected above, please enter in a numeric day value. Results will be retrieved relative to the current date and the value you enter."
MOD_ARTICLES_CATEGORY_FIELD_STARTDATE_DESC="If Date Range is selected above, please enter a Starting Date."
 

`

avatar brianteeman
brianteeman - comment - 18 Jun 2018

Sorry not wasting any more time on this - there are far too many unrelated changes to make testing sensible and its impossible to follow comments. I can see there is some good stuff here but I also see other changes which have impact elsewhere.

This is why you must have one pr per issue

avatar SharkyKZ
SharkyKZ - comment - 22 Jun 2018

Okay, going to try break this up. Closing for now.

avatar SharkyKZ SharkyKZ - change - 22 Jun 2018
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2018-06-22 08:45:45
Closed_By SharkyKZ
avatar SharkyKZ SharkyKZ - close - 22 Jun 2018

Add a Comment

Login with GitHub to post a comment