? Success

User tests: Successful: Unsuccessful:

avatar Harmageddon
Harmageddon
6 Oct 2015

This is a redo of #3630, using the current staging branch and with some improvements.

Contents

This PR implements the following plugin events for plugins (group: content):

Event Views
onContentPrepare
  • contact
  • category (for category title, category description and every contact)
onContentAfterTitle
  • contact
  • category (for every contact)
onContentBeforeDisplay
  • contact
  • category (for every contact)
onContentAfterDisplay
  • contact
  • category (for every contact)

Testing instructions

  1. Install the patch.
  2. If you don't have any plugin installed that targets com_contact, nothing should change in any view (especially in the com_contact views).
  3. Install a content plugin that uses the events mentioned above in com_contact. You can either write a new one, modify one of the existing content plugins, or use this small plugin for testing purposes.
  4. Make sure that the plugin works in the desired way.
  5. Please test Beez (default and encyclopedia layout), too!
avatar Harmageddon Harmageddon - open - 6 Oct 2015
avatar Harmageddon Harmageddon - change - 6 Oct 2015
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 6 Oct 2015
Labels Added: ?
avatar Harmageddon Harmageddon - change - 6 Oct 2015
Category Components Plugins
avatar Harmageddon Harmageddon - change - 6 Oct 2015
The description was changed
avatar Harmageddon
Harmageddon - comment - 6 Oct 2015

Updated the PR with Beez support and a slightly different context for category descriptions.


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

avatar wilsonge
wilsonge - comment - 6 Oct 2015

Thankyou :) I'll try and find some time to test over the next week or so. But can't guarantee anything :(

avatar zero-24 zero-24 - change - 6 Oct 2015
Easy No Yes
avatar Rivaner
Rivaner - comment - 24 Oct 2015

Testing works!


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

avatar SniperSister SniperSister - alter_testresult - 24 Oct 2015 - Rivaner: Tested successfully
avatar KlausKrippeit KlausKrippeit - test_item - 24 Oct 2015 - Tested successfully
avatar KlausKrippeit
KlausKrippeit - comment - 24 Oct 2015

I have tested this item :white_check_mark: successfully on ccb17b0

@test successfully tested.


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

avatar waader
waader - comment - 24 Oct 2015

@test works for me too. Also saw some improvements. Thanks for that!

avatar yvesh yvesh - test_item - 24 Oct 2015 - Tested successfully
avatar yvesh
yvesh - comment - 24 Oct 2015

I have tested this item :white_check_mark: successfully on ccb17b0

@Test works fine


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

avatar wilsonge wilsonge - change - 24 Oct 2015
Status Pending Ready to Commit
avatar wilsonge
wilsonge - comment - 24 Oct 2015

RTC


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

avatar joomla-cms-bot joomla-cms-bot - change - 24 Oct 2015
Labels Added: ?
avatar zero-24
zero-24 - comment - 24 Oct 2015

@Harmageddon can you fix the merge conflicts.

avatar zero-24 zero-24 - change - 24 Oct 2015
Milestone Added:
avatar joomla-cms-bot
joomla-cms-bot - comment - 24 Oct 2015

This PR has received new commits.

CC: @KlausKrippeit, @Rivaner, @yvesh


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

avatar Harmageddon
Harmageddon - comment - 24 Oct 2015

@zero-24 Should be fine now.

avatar zero-24
zero-24 - comment - 24 Oct 2015

Thanks @Harmageddon

avatar wilsonge wilsonge - reference | 88fb40b - 26 Oct 15
avatar roland-d
roland-d - comment - 26 Oct 2015

Thanks everybody, this has now been merged into 3.5-dev with commit 88fb40b

Gosh what a brainkiller this was to merge :P

avatar roland-d roland-d - change - 26 Oct 2015
Status Ready to Commit Closed
Closed_Date 0000-00-00 00:00:00 2015-10-26 19:50:34
Closed_By roland-d
avatar roland-d roland-d - close - 26 Oct 2015
avatar joomla-cms-bot joomla-cms-bot - close - 26 Oct 2015
avatar roland-d roland-d - close - 26 Oct 2015
avatar joomla-cms-bot joomla-cms-bot - change - 26 Oct 2015
Labels Removed: ?
avatar mbabker
mbabker - comment - 2 Dec 2015

This pull request has introduced two regressions:

1) Via 88fb40b#diff-075095e768ef950cf2dcc5f08cf5436aR148 plugin events are now being triggered twice for all items on the com_content.category view
2) 88fb40b#diff-075095e768ef950cf2dcc5f08cf5436aR159 is hardcoded to com_contact.category as the context instead of correctly determining the extension as is done with every other trigger

avatar Bakual
Bakual - comment - 2 Dec 2015

@mbabker You know what comes. Can you open a new issue (or even PR) that references to this PR?
I'm sure this will get lost otherwise.

avatar wilsonge
wilsonge - comment - 2 Dec 2015

#8581 PR for the context (part 2). Don't have time right at this second to look into number 1. Will do that later.

avatar mbabker
mbabker - comment - 2 Dec 2015

Can you open a new issue (or even PR) that references to this PR?

If I do it's taking the event triggers out of JViewCategory because it's damn near a B/C issue (extensions extending it and calling that method are now either getting new plugin events that weren't in place originally or in the case of com_content now causes events to be triggered twice).

This pull request should not have been merged without testing against ALL components because of the changes in JViewCategory at a minimum. Also, the actual merge for this pull request (commit 88fb40b) is so vastly different than the state this pull request was accepted in at this point I would practically demand that the original merge be reverted and the merge attempted again without all the extra changes (compare the changes for components/com_contact/views/contact/tmpl/default.php from this pull request with the committed changes. The diffs for these files alone do not align, not to mention the number of files changed in this pull request compared to the actual merge.

avatar mbabker mbabker - reference | 5c19acf - 2 Dec 15
avatar mbabker mbabker - reference | f92ab2b - 2 Dec 15
avatar mbabker mbabker - reference | 9a3c483 - 2 Dec 15
avatar mbabker mbabker - reference | 4c7c417 - 2 Dec 15
avatar mbabker mbabker - reference | 3c968ff - 2 Dec 15
avatar ggppdk
ggppdk - comment - 23 Jan 2016

Any update on best way to fix the double event triggering ?

  • will triggering be removed from the JViewCategory class (and then the other relevant changes that depend on it) ?
  • or all views like ContentViewCategory (extends JViewCategory) and other views in 3rd party extensions, will need to remove the triggering ?

Also 3 out of the 4 triggers are using ->core_params which does not exist
and 1 is using ->params that does exist

https://github.com/joomla/joomla-cms/blob/staging/libraries/legacy/view/category.php#L161-L168

$results = $dispatcher->trigger('onContentAfterTitle', array($this->extension . '.category', &$itemElement, &$itemElement->core_params, 0));
...
$results = $dispatcher->trigger('onContentBeforeDisplay', array($this->extension . '.category', &$itemElement, &$itemElement->core_params, 0));
...
$results = $dispatcher->trigger('onContentAfterDisplay', array($this->extension . '.category', &$itemElement, &$itemElement->core_params, 0));
...

Regarding the empty parameters, there is a report for it here:
#8963

avatar ggppdk
ggppdk - comment - 23 Jan 2016

ok, my comment about 3rd party extensions is mostly wrong, only a very small number (if any) will need to be updated

because function commonCategoryDisplay which got the new code to trigger plugin event is inside JViewCategory and not inside JViewLegacy, and then double triggering is only if you call parent::commonCategoryDisplay();

avatar wilsonge
wilsonge - comment - 23 Jan 2016

I'm so sorry I forgot about this. I'm out today but will try and get back on this either this evening or tomorrow. Please ping me repeatedly on this if it feels like I've forgotten about the issue!

avatar ggppdk
ggppdk - comment - 25 Jan 2016

Besides fixing $itemElement->core_params (which does not exist) to be ->params

A simple and clean fix for the double category triggering is

  • add 1 parameter to the signature of method

public function commonCategoryDisplay()

at libraries/legacy/view/category.php (class JViewCategory) so as to be:

public function commonCategoryDisplay($trigger_events=false)


Then update views that extend JCategoryView (and that also call commonCategoryDisplay) and for which default code for triggering events is good enough:
com_weblinks\views\category\view.html.php
com_newsfeed\views\category\view.html.php
com_contact\views\category\view.html.php

parent::commonCategoryDisplay($trigger_events=true);

And then views that need a custom preparation for plugin event triggering and want to do it inside their own code:
com_content\views\category\view.html.php

parent::commonCategoryDisplay($trigger_events=false);
avatar ggppdk
ggppdk - comment - 25 Jan 2016

Also since plugin event trigger was added to the parent class (JViewCategory),

  • adding this parameter is rather a requirement ??

otherwise it is rather inflexible, and any view wanting to do some extra preparation for plugin event triggering will need to skip the commonCategoryDisplay altogether and copy all its code inside the current view

plus it becomes background compatible

avatar danyj
danyj - comment - 30 Jan 2016

Testing beta 2 this is still an issue #8963

avatar roland-d
roland-d - comment - 10 Feb 2016

Please ping me repeatedly on this if it feels like I've forgotten about the issue!

@wilsonge Pinging you as the discussion is still on-going.

avatar wojsmol
wojsmol - comment - 14 Feb 2016

Testing on latest staging double event triggering is still an issue see backbutton/content-plugin#6

avatar wojsmol
wojsmol - comment - 15 Feb 2016

@wilsonge Pingig you regardig double event triggering.

avatar wilsonge
wilsonge - comment - 15 Feb 2016

Thankyou! I have left this up on my laptop and will have code to fix it on the way home from work this evening!

avatar wilsonge
wilsonge - comment - 18 Feb 2016

Fixed with fb4d375

Add a Comment

Login with GitHub to post a comment