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

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.

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

$item_heading restored.

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.

Before patch

After patch

@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"

@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)

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...

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

@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.

@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.

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.

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.

@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.

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

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

@Quy
I do not get any error with staging

@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.

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

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.

Nevermind. The issue occurs even without this PR.

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

@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.

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>

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.

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

Its easier to test one by one.

@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.

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

@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.

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."
 

`

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

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