Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes the double event firing #18066

Merged
merged 3 commits into from Sep 26, 2017
Merged

Conversation

@laoneo
Copy link
Member

@laoneo laoneo commented Sep 22, 2017

Pull Request for Issue #18016.

Summary of Changes

Prepares the intro text within the same prepare event. Removes double onContentPrepare event triggering.

Testing Instructions

See #17175 and #18016.

Expected result

No twice event triggering and field is rendered in intro text as well.

Actual result

Side effects like modules get loaded twice.

@ghost
Copy link

@ghost ghost commented Sep 22, 2017

@laoneo which is the second Issue you wanted to link (you take #18016 twice)? Guess ##18065

@laoneo
Copy link
Member Author

@laoneo laoneo commented Sep 22, 2017

Thanks for the hint, changed it in the description.


if ($matches)
if (!$matches)

This comment has been minimized.

@regularlabs

regularlabs Sep 22, 2017
Contributor

probably better to use if(empty($matches)) here.

}

private function prepare($string, $context, $item)
{
// Search for {field ID} or {fieldgroup ID} tags and put the results into $matches.
$regex = '/{(field|fieldgroup)\s+(.*?)}/i';

This comment has been minimized.

@regularlabs

regularlabs Sep 22, 2017
Contributor

If you change the regex to:

$regex = '/{(?<type>field|fieldgroup)\s+(?<id>.*?)}/i';

Then you can reference these as $match['type'] and $match['id'] instead of $match[1] and $match[2].
This will make the code a lot more readable.

This comment has been minimized.

@laoneo

laoneo Sep 22, 2017
Author Member

We can do that on a second step. First lets fix the bug.

)
);
}
}

This comment has been minimized.

@regularlabs

regularlabs Sep 22, 2017
Contributor

If you add a continue; here, you don't need the else.

@regularlabs
Copy link
Contributor

@regularlabs regularlabs commented Sep 22, 2017

The issue with the double triggering of the onContentPrepare is fixed.
I have not tested if the custom fields plugin itself still works as it should.

@laoneo
Copy link
Member Author

@laoneo laoneo commented Sep 22, 2017

Please mark it as success. Your code improvements suggestions should be done on a follow up pr as I don't want to get the risk of a new bug.

@sandewt
Copy link
Contributor

@sandewt sandewt commented Sep 22, 2017

I have tested this item successfully on e8e0329

Article View that was opened via a link from Article Category List: Module loaded twice #18016

Plugin content pagebreak does not work for second page and next #17830


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

For clarity: I tested #18066 Fixes the double event firing
Joomla! 3.8.1-dev Development
PHP 7.0.15
Xampp

@laoneo
Copy link
Member Author

@laoneo laoneo commented Sep 23, 2017

Fixed only some code styles. Guess this can be marked as RTC as we have two successful tests.

@n9iels
Copy link
Contributor

@n9iels n9iels commented Sep 25, 2017

I have tested this item successfully on 366588c


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

@ghost
Copy link

@ghost ghost commented Sep 25, 2017

@sandewt can you please test again?

@wojsmol
Copy link
Contributor

@wojsmol wojsmol commented Sep 26, 2017

@franz-wohlkoenig IMHO as 366588c add only dockblock we can set RTC with one good test from @nibra .

@ghost
Copy link

@ghost ghost commented Sep 26, 2017

RTC after successful tests. As always Release Lead decide if it get merged.

@wojsmol Thanks for Hint.

@joomla-cms-bot joomla-cms-bot added the RTC label Sep 26, 2017
@Bakual
Copy link
Contributor

@Bakual Bakual commented Sep 26, 2017

Doesn't that mean that when the field content plugin is disabled, no events are processed anymore?
Doesn't look right to me.

@n9iels
Copy link
Contributor

@n9iels n9iels commented Sep 26, 2017

@Bakual is right. Unless there is another point where onContentPrepare is called, this event won't be fired if the fields plugin is disabled

@ghost
Copy link

@ghost ghost commented Sep 26, 2017

remove RTC?

@regularlabs
Copy link
Contributor

@regularlabs regularlabs commented Sep 26, 2017

Isn't the whole idea for this change to trigger the fields plugin over the required article data?
So it makes sense that this won't (have to) do anything in the fields plugin is not enabled.

The initial onContentPrepare trigger is still there.
So what events do you mean?
https://github.com/joomla/joomla-cms/pull/18066/files#diff-a2fbd25d7bac224516450abdd42cd57bL190

@Bakual
Copy link
Contributor

@Bakual Bakual commented Sep 26, 2017

@regularlabs You're right, it only reverts the introtext part added in a previous PR.
Still looks a bit wrong to me to have com_content specific stuff in the fields plugin.

@regularlabs
Copy link
Contributor

@regularlabs regularlabs commented Sep 26, 2017

Well, that's a bit beyond this PR to discuss that.
But the way the onContentPrepare event (and other content triggers) work in Joomla pretty much forces plugins to check for com_content related stuff. This is because the full $item object is passed to the event, not a simple string. Not saying that is a bad thing, as it makes it a lot more flexible. But it does mean the plugins need to decide on what $item values it wants to do stuff on.
If you want to also be able to use the fields plugin in category descriptions - for instance - then the plugin would have to do its thing specifically on the $item->description value too.

@Bakual
Copy link
Contributor

@Bakual Bakual commented Sep 26, 2017

True enough.

@mbabker mbabker added this to the Joomla 3.8.1 milestone Sep 26, 2017
@mbabker mbabker merged commit 1ed272f into joomla:staging Sep 26, 2017
4 of 5 checks passed
4 of 5 checks passed
JTracker/HumanTestResults Human Test Results: 1 Successful 0 Failed.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound No violations found. Woof!
@joomla-cms-bot joomla-cms-bot removed the RTC label Sep 26, 2017
@laoneo laoneo deleted the Digital-Peak:j3/fix/introtext branch Sep 26, 2017
@regularlabs
Copy link
Contributor

@regularlabs regularlabs commented Sep 26, 2017

Great 👍

@regularlabs
Copy link
Contributor

@regularlabs regularlabs commented Sep 26, 2017

@mbabker Just to throw this in perspective:
IMHO this is a very important fix that should be rolled out asap.
Without the fix, websites can potentially load a lot slower and even break (which is already happening to many).
So upgrading to Joomla 3.8 is currently detrimental for many websites.

@mbabker
Copy link
Contributor

@mbabker mbabker commented Sep 26, 2017

Well aware of that. I'm trying to hold off though on a 3.8.1 until we at least have a grasp on everything that's being reported so I'm not spending 10-15 hours per week prepping and rolling releases until it's all sorted. Depending on how things go the rest of this week we can probably do 3.8.1 next week.

@sandewt
Copy link
Contributor

@sandewt sandewt commented Sep 27, 2017

"@sandewt can you please test again?" Request from @franz-wohlkoenig

I see in the meantime that the code is merged.
I tested the issues #18016, #17830 and #17175 with success in the latest version of joomla-staging-cms.

But I see sometimes, by using some other content (!), that the menus in Protostar (position 7 - at the right) disappeared to the bottom !?
I can not explain this.

Is that a known or a new issue?


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18066.
@ghost
Copy link

@ghost ghost commented Sep 27, 2017

@sandewt can you please open a new Issue?

@sandewt
Copy link
Contributor

@sandewt sandewt commented Sep 28, 2017

@franz-wohlkoenig

Some text in articles I used, were 'accidentally' embedded by div's. In combination with NOT 'show the intro text', causes that the menus disappeared to the bottom. So I can explain this behavior.

In my opinion, it is not a new issue.


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

@ghost
Copy link

@ghost ghost commented Sep 28, 2017

its about getting Notice as this PR is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.