$params should be a JRegistry object. This may well cause issues with content plugins trying to access properties. So that's not a solution.
A proper solution would be to set a proper context. Like $extension .'.category.title' and $extension .'.category.description'.
Or maybe only trigger onContentPrepare once instead twice.

This may however cause issues with existing plugins as well, but at least it would be a proper fix for the issue.

Yes, I agree that ...category.title would be ideal. I wonder how much it would break.

I could make and pass it a JRegistry object with 'content_type' => 'title' (or something similar). What do you think of that?

Personally, I would prefer a proper context over misusing the params.

Ok, if I change my pull request to use $extension.'.category.title' for the first call, I think that should not be too upsetting to existing plugins since that callback was new in Joomla 3.1 or 3.2 and I doubt many plugins are making use of it.

So if you guys agree I'll change it.

I thought some more about it and changed the category list title content.prepare invocation to use $extension.category.title. And I updated the pull request. I did not modify the content.prepare invocation for category descriptions. I believe this is the correct approach.

I also think that this approach should not be a problem for existing plugins for these reasons:

• It does not affect the callback for category descriptions, so existing content plugins that do something for the category descriptiion (in onContentPrepare callbacks) should not be affected.

• Any content plugin actually changing the title already has a problem since they have no way to reliably determine if their onContentPrepare callback has been called for a category title or description. Their callbacks may have lucked out and worked in the cases of hidden or empty category descriptions, but would have failed if both category titles and descriptions were displayed. This may mean that a rare few content plugins may need to be updated, but they were probably broken anyway, even if they did not realize it.

• This callback (for the title) was only added in Joomla 3.1 or 3.2, so hopefully not many content plugins are using it yet.

I have not looked for other instances of this in the Joomla code, but it is possible there are others.

-Jonathan

I like that approach

@bakual
What do I need to do to get this change accepted? I could create a simple test plugin that adds a text tag to the category title and description, and then demonstrate that with the fix the plugin can tell the difference between the category description and category title. But that seems pretty obvious and I'm not sure it really needs that kind of test.

I can ask a couple of users to test the (Attachments) extension fix that I've implemented, but in essence they would be testing the fix plus my extension and I'm not sure that type of testing would be acceptable.

Or we could accept this on merit--on the obviousness of the argument that there is no way to robustly tell the difference in that layout between a category title and the category description in onContentPrepare callbacks without this fix.

-Jonathan

P.S. [EDIT] I had made a comment that the $row could be checked for the title to disambiguate the category title from the description--but I was wrong about that. the $row object is only the actual text of the title or description, hence there really is now way to distinguish the two calls!

I've set the tracker to pending, looks like nobody did so far.
We usually need two good tests before we merge anything. I don't care who they are, as long as they test the patch and also look a bit further around than just testing your extension.
Best is always to search for ways to break the patch. If nothing is found, the patch is good

Thanks Thomas. I'll see what I can do. Most of the folks doing bug-fix testing are not interested in this type of patch since it does not affect their extension (if they have one they have developed). I'll try to recruit some new blood for the test.

Of course, with my own patch, I would be happy to have it accepted because its correctness is obvious! But I would not accept that for anyone else's patch! So on to testing...

-Jonathan

I have 15 PRs pending, I know the feeling

I am willing to test the new changes to the Attachments extension. I have multiple test sites I an use...

@bgraftan Excellent. I'll put together a test plugin this weekend. Thanks.

I have created a plugin to illustrate this issue and show that this patch fixes the problem. You see it at this github project:

https://github.com/jmcameron/category_list_test

The README on this page also shows the full testing procedure.

-Jonathan

Worked like a charm....please see the attached images as I stepped through the directions to the end result.

@bgrattan I noticed in the pre-fix screenshot that the category title did not have [CATEGEGORY DESCRIPTION?] appended to it. I presume that is because the template you are using is handling the title differently than the default one? But the final screen shot does show that the fix is working.

That is correct...and at that time I had not converted it from category blog to category list. If you need I can provide the necessary image as evidence.

Now that you have mentioned that here, I do not think adding the image is
necessary. But note that you can edit previous posts, if you wish.

-Jonathan

On Sat, Feb 15, 2014 at 9:48 AM, bgrattan notifications@github.com wrote:

That is correct...and at that time I had not converted it from category
blog to category list. If you need I can provide the necessary image as
evidence.

Reply to this email directly or view it on GitHub#2842 (comment)
.

Tested and was successful.

@Bakual @mbabker We now have two tests of this fix. What do you think about accepting it?

Thanks
-Jonathan

No objections from me of course.
Set tracker to RTC.

Very nice, but the page title (I mean the one on the header) shouldn't also be the same. Maybe we should also set the page title with the H1 text, for better SEO.

$document->setTitle('the same as <h1>');

I do not know whether the category should switch to H1 or not. But I think that this patch is independent of that issue.

As far as I can tell, the goal of the onContentPrepare callback for category titles is to modify the title somehow; the result should print however the category title is normally printed (in terms of H levels, etc).

-Jonahtan

I think you are absolutely right, I was confused watching the last image of @temery where the window title is test and the h1 is Test [Category title!], but this most probably comes from the menu...

Indeed it does.....

Sent from my iPhone

On Feb 17, 2014, at 12:13 PM, dgt41 notifications@github.com wrote:

I think you are absolutely right, I was confused watching the last image of @temery where the window title is test and the h1 is Test [Category title!], but this most probably comes from the menu...

—
Reply to this email directly or view it on GitHub.

Could you also correct Beez3?
Line 36 of /templates/beez3/html/com_content/category/default.php
something like:
echo '<span class="subheading-category">' . JHtml::_('content.prepare', $this->category->title, '', 'com_content.category.title') . '</span>';

and I guess same for blog.php line 29

I did a search and found these occurences. Should I fix them all?

components/com_newsfeeds/views/category/tmpl/default.php:26:
templates/beez3/html/com_weblinks/category/default.php:23:
templates/beez3/html/com_contact/category/default.php:21:
templates/beez3/html/com_newsfeeds/category/default.php:23: 
layouts/joomla/content/category_default.php:35:  [ALREADY FIXED IN THIS PATCH]

-Jonathan

I also did a search of the Joomla 2.5 branch and find three occurrences (similar to the com_weblinks, com_contacts, and com_newsfeeds items above but not associated with a specific template. If you like I can put together a separate pull request for that.

-Jonathan

For the sake of consistency, I think it would make sense.

I'm not a big fan of calling JHtmlContent::prepare() on the title. If a plugin wants to target the title, it could already do that by checking $item->title during the first call (in the view itself).
But since it is already there, let's at least do it right.

Ok, I've added these fixes to the other four locations in my Joomla 3.x patch in this PR (see the files changed above for details).

Once this PR is accepted and merged, I'll do another one for the Joomla 2.5 side.

I agree that it does not make a lot of sense to me to be doing onContentPrepare for category titles, but someone added it for a reason. It is possible that some extensions that are using this callback may need updating. But it only seems proper since I cannot imagine how they could have been using it correctly before!

-Jonathan

Looks like your forgot beez 3 content/category/default.php and blog.php as I posted above

I just checked the template files beez 3 content/category/default.php and
blog.php -- and both of these files only have content.prepare callbacks for the
category description, not the category title (which is what we are trying
to fix). So I do not think either of these files need updating.

Please double check and if I missed something, please point out the line
number(s) so I can see exactly what you are referring to.

Thanks

-Jonathan

On Wed, Feb 19, 2014 at 4:16 AM, infograf768 notifications@github.comwrote:

Looks like your forgot beez 3 content/category/default.php and blog.php as
I posted above

Reply to this email directly or view it on GitHub#2842 (comment)
.

I just double-checked by doing a global search and I'm pretty sure I've fixed all the content.prepare callbacks for category titles.

-Jonathan

@jmcameron
No reason why we would have the callback in core and not in beez3 (it was obviously forgotten), thus why I asked them to be added there.

@infograf768
I thought you wanted me to add the '.title' to the context for category list titles content.prepare callbacks, but as far as I can tell, these are currently missing in beez3. So you're asking me to add the same content.prepare callback for category list titles in beez3 as is in the core, right? I wanted to double-check these before doing this modification, since that goes a bit beyond the scope of this pull request. I'm happy to do it, but I want to make sure I understand what you're asking me to do.
-Jonathan

On Wed, Feb 19, 2014 at 11:19 PM, infograf768 notifications@github.comwrote:

@jmcameron https://github.com/jmcameron
No reason why we would have the callback in core and not in beez3 (it was
obviously forgotten), thus why I asked them to be added there.

Reply to this email directly or view it on GitHub#2842 (comment)
.

@infograf768 Ok, somehow I missed your post where you suggested I correct Beez3 to add the missing callbacks. Correction to my last post: The two instances you pointed are the two locations where the title is not processed by content.prepare. Which is what you wanted me to do. I just did it and updated my PR. So please check the file diffs and make sure it was what you intended.

I re-ran my tests using the beez3 template and it worked properly with this fix.

Sorry about the confusion.
-Jonathan

Thanks. We need now a couple of testers for this last iteration of the PR

@jmcameron - I updated your patch to include contexts for com_newsfeeds, com_weblinks and com_contact so it could fully test the Beez3 changes. Have tested the category views of the 4 components in Protostar and Beez3 and confirm the patch works as expected.

@infograf768 I noticed that Simonet tested this too (see the tracker ticket).

-Jonathan

@radiant-tech Thanks for testing! Do I need to update the code changes in my PR?

-Jonathan

The PR is good. I just change the code of your plugin in my local working
directory to make the testing clear.

public function onContentPrepare($context, &$row, &$params, $page = 0)
{
    $title_array = array('com_content.category.title',

'com_newsfeeds.category.title', 'com_contact.category.title',
'com_weblinks.category.title');
if (in_array($context, $title_array))
{
$row->text .= " [CATEGORY TITLE!]";
return true;
}

    $desc_array = array('com_content.category',

'com_newsfeeds.category', 'com_contact.category', 'com_weblinks.category');
if (in_array($context, $desc_array))
{
$row->text .= " [CATEGORY DESCRIPTION?]";
return true;
}

    return false;
}

On Sat, Feb 22, 2014 at 12:05 PM, Jonathan Cameron <notifications@github.com

wrote:

@radiant-tech https://github.com/radiant-tech Thanks for testing! Do I
need to update the code changes in my PR?

-Jonathan

Reply to this email directly or view it on GitHub#2842 (comment)
.

@radiant-tech Do you want to do a pull request against my test (https://github.com/jmcameron/category_list_test)?

-Jonathan

Sent the PR Jonathan. Good work on this fix.

On Sat, Feb 22, 2014 at 12:26 PM, Jonathan Cameron <notifications@github.com

wrote:

@radiant-tech https://github.com/radiant-tech Do you want to do a pull
request against my test (https://github.com/jmcameron/category_list_test)?

-Jonathan

Reply to this email directly or view it on GitHub#2842 (comment)
.

Thanks Denise, I've updated the ZIP file in case others want to try it.

It was exciting merging my very first pull request against one of my projects on github!

-Jonathan

@jmcameron
I am Jean-Marie Simonet... ;)

@infograf768 Thanks for the introduction!

-Jonathan

Thanks!