To avoid Fatal error with JEditor::getButtons (for issue #660) #1093

Closed
wants to merge 11 commits into
from

Conversation

Projects
None yet
6 participants
Contributor

Buddhima commented Apr 1, 2012

File: joomla-platform / libraries / joomla / html / editor.php
Method: getButtons()

Issue: If the $className would not exists (due to plugin class is missing) , assigning to $temp variable becomes unsafe

Solution: Put the assiging inside a try catch block, so that not existance of plugin class would not disturb the flow of foreach loop

Signed-off-by: Buddhima Wijeweera

@Buddhima Buddhima for issue #660
To avoid unsafe operation

Signed-off-by: Buddhima Wijeweera <buddhimawijeweera@yahoo.com>
532ca47
Owner

mbabker commented Apr 1, 2012

You need to fix your code style first - http://developer.joomla.org/pulls/pulls/1093.html

Also, instead of echoing the message, you should be returning an error. Instead of the try/catch method you've proposed, maybe the current if statement would work fine, but add an else so that an Exception is thrown and the caller can handle the error.

@elinw elinw and 1 other commented on an outdated diff Apr 1, 2012

libraries/joomla/html/editor.php
// Try to authenticate
- if ($temp = $plugin->onDisplay($editor, $this->asset, $this->author))
- {
- $result[] = $temp;
- }
+ // Unsafe opearation, might cause an exception if $className not exists
@elinw

elinw Apr 1, 2012

Contributor

operation

@Buddhima

Buddhima Apr 1, 2012

Contributor

:D a mistake. Thanks for pointing out
Corrected !!!

Buddhima added some commits Apr 1, 2012

@Buddhima Buddhima commit 2 after applying coding standards. Thanks mbabker!
Signed-off-by: Buddhima Wijeweera <buddhimawijeweera@yahoo.com>
af7d4c3
@Buddhima Buddhima corrected "operation" spellings
Signed-off-by: Buddhima Wijeweera <buddhimawijeweera@yahoo.com>
deb68e1
@Buddhima Buddhima Throwing Exception instead of try-catch block
Signed-off-by: Buddhima Wijeweera <buddhimawijeweera@yahoo.com>
a89332b
@Buddhima Buddhima changed Exception to InvalidArgumentException, added @throws in docblock
Signed-off-by: Buddhima Wijeweera <buddhimawijeweera@yahoo.com>
0bd1796
@Buddhima Buddhima used single quotes and tabs instead of spaces
Signed-off-by: Buddhima Wijeweera <buddhimawijeweera@yahoo.com>
74b3027
@Buddhima Buddhima changed quotes and added tabs
Signed-off-by: Buddhima Wijeweera <buddhimawijeweera@yahoo.com>
b66f8c6
@Buddhima Buddhima removed spaces and replaced with tabs in latter section
Signed-off-by: Buddhima Wijeweera <buddhimawijeweera@yahoo.com>
24ed611
@Buddhima Buddhima Corrected error in
libraries/joomla/html/editor.php:470

Signed-off-by: Buddhima Wijeweera <buddhimawijeweera@yahoo.com>
434f00e

@chdemko chdemko commented on an outdated diff Apr 13, 2012

libraries/joomla/html/editor.php
@@ -426,6 +426,8 @@ public function setContent($editor, $html)
*
* @return array
*
+ * @throws InvalidArgumentException If plugin required does not exists
+ *
@chdemko

chdemko Apr 13, 2012

Contributor

Please indent correctly

@chdemko chdemko commented on an outdated diff Apr 13, 2012

libraries/joomla/html/editor.php
// Try to authenticate
if ($temp = $plugin->onDisplay($editor, $this->asset, $this->author))
{
$result[] = $temp;
}
+
@chdemko

chdemko Apr 13, 2012

Contributor

superfluous line

Buddhima added some commits Apr 14, 2012

@Buddhima Buddhima Indent settled correctly and remove superfluous line
Signed-off-by: Buddhima Wijeweera <buddhimawijeweera@yahoo.com>
dc37ef8
@Buddhima Buddhima Indent settled correctly
Signed-off-by: Buddhima Wijeweera <buddhimawijeweera@yahoo.com>
dfc11f3
Contributor

chdemko commented Jun 14, 2012

The pull request cannot be merged

Contributor

Buddhima commented Jun 21, 2012

Hi, can you tell me why it is failing?

Contributor

chdemko commented Jun 21, 2012

you have to merge with the current trunk

Contributor

elinw commented Jun 21, 2012

The issue is not that the pull request can't be merged (that would be shown on the pull tester report with red) All of the pull requests with ?? in them have the same failure. I'm not sure if it is because they all have a dependency on JHtml or not, but that is possible. Those test reports were generated on May 5 and have not been updated since.
All of them show this as the failing line
https://github.com/joomla/joomla-platform/blob/staging/tests/suites/unit/joomla/html/JHtmlTest.php#L896

Contributor

Buddhima commented Jun 22, 2012

Thanks Elin for pointing to the correct place :)

Contributor

elinw commented Jun 23, 2012

Okay now you need to update your branch, since the pulltester has rerun and it says it is not able to merge cleanly.

Member

realityking commented Jul 18, 2012

This still doesn't merge cleanly. Also I'm wondering under what circumstances this would be triggered.

Contributor

elinw commented Oct 9, 2012

@realityking if the plugin is missing but you put the button in the field definition, I believe.

@Buddhima will you please rebase this or close it until you get it rebased?

Contributor

LouisLandry commented Oct 9, 2012

@Buddhima the library in question has been removed from the platform and added to the CMS tree. I'd suggest you submitting this to the CMS repository since it is a very smart and correct change in behavior. I'm closing this since it is no longer relevant for the platform. Apologies for not getting to it sooner.

LouisLandry closed this Oct 9, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment