Skip to content
This repository has been archived by the owner on Nov 26, 2017. It is now read-only.

Fixed issue with JFormHelper:loadClass() including only first filename match #460

Merged
merged 4 commits into from Nov 4, 2011
Merged

Conversation

ghost
Copy link

@ghost ghost commented Oct 23, 2011

Now it includes all matched files till it finds the required classname. Fixes #455

Peter van Westen added 3 commits October 23, 2011 15:10
…e match. Now it includes all matched files till it finds the required classname.
@eddieajau
Copy link
Contributor

Peter, I think all we need is the unit test to validate the changes. Thanks in advance.

@ghost
Copy link
Author

ghost commented Oct 26, 2011

Great. Just to add:
This is just a quickfix till this whole thing is handled in a better lower-level way as mentioned by Louis (using JLoader).
So it might be good to add something along those lines in the comment near this code block...

@joomla-jenkins
Copy link

Unit testing complete. There were 0 failures and 1 errors from 1683 tests and 10557 assertions.
Checkstyle analysis reported 453 warnings and 6 errors.

@ghost
Copy link
Author

ghost commented Nov 1, 2011

Any info on what the errors are? Anything I can/should do?

@elinw
Copy link
Contributor

elinw commented Nov 1, 2011

Peter do you have phpunit installed? You can run the locally. There are instrutions on the wiki and here
https://github.com/joomla/joomla-platform/blob/staging/docs/manual/appendices/analysis.xml.

@ghost
Copy link
Author

ghost commented Nov 1, 2011

No, I haven't. Pretty new to this. So I'll look into it...

@joomla-jenkins
Copy link

Build triggered by changes to the base.

Unit testing complete. There were 0 failures and 0 errors from 1683 tests and 10559 assertions.
Checkstyle analysis reported 453 warnings and 2 errors.

@ghost
Copy link
Author

ghost commented Nov 2, 2011

So all is ok?

The Checkstyle warnings/errors, does that concern these changes? Can't imagine these few lines generating 453 warnings :S

@elkuku
Copy link
Contributor

elkuku commented Nov 2, 2011

The warnings come from all over the place - the errors shouldn't ;)

Checkstyle error details:
libraries/joomla/form/helper.php:202
Concat operator must be surrounded by spaces
libraries/joomla/form/helper.php:203
Expected "foreach (...)\n...{\n"; found "foreach(...)\n...{\n"

@joomla-jenkins
Copy link

Build triggered by changes to the head.

The tests completed but there was a problem parsing the report.
Unit testing complete. There were 0 failures and 0 errors from 1742 tests and 10618 assertions.
Checkstyle analysis reported 453 warnings and 29 errors.

@ghost
Copy link
Author

ghost commented Nov 3, 2011

Sorry, I don't understand. Is everyone using different coding styles?

@elinw
Copy link
Contributor

elinw commented Nov 3, 2011

The 453 are the same ones everyone has and have nothing to do with your code. But if the numbers go up it's due to your code.

@ghost
Copy link
Author

ghost commented Nov 3, 2011

Yes, but what about the 29 errors?
"Checkstyle analysis reported 453 warnings and 29 errors."

@elkuku
Copy link
Contributor

elkuku commented Nov 3, 2011

I believe, that's a mistake. From my POV your code is OK
Please be patient ;)

@ghost
Copy link
Author

ghost commented Nov 3, 2011

Ok, great. I am patient. Just worried I am doing stuff wrong. :)

@LouisLandry
Copy link
Contributor

Best I can tell this is all good. Going to merge it now. Thanks Peter.

LouisLandry added a commit that referenced this pull request Nov 4, 2011
Fixed issue with JFormHelper:loadClass() including only first filename match
@LouisLandry LouisLandry merged commit 7a35705 into joomla:staging Nov 4, 2011
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

loadElement only includes first matched element file. Problem with multiple elements with same main name.
5 participants