This repository has been archived by the owner. It is now read-only.

sprintf complains unless the correct number of arguments are passed #1720

Merged
merged 1 commit into from Nov 29, 2012

Conversation

Projects
None yet
5 participants
@okonomiyaki3000
Contributor

okonomiyaki3000 commented Nov 27, 2012

An undefined variable was being passed to JText::sprintf in two places. This was fixed a little while ago, in one case it was replaced but in another it was omitted. Since sprintf throws a warning when the format uses numbered arguments but insufficient arguments are passed to the function... we need to pass something. Luckily, there is already a suitable string: JLIB_APPLICATION_ERROR_COMPONENT_NOT_FOUND

@eddieajau

View changes

libraries/legacy/component/helper.php
@@ -397,7 +397,7 @@ protected static function _load($option)
if (empty(self::$components[$option]))
{
// Fatal error.
- JLog::add(JText::sprintf('JLIB_APPLICATION_ERROR_COMPONENT_NOT_LOADING', $option), JLog::WARNING, 'jerror');
+ JLog::add(JText::sprintf('JLIB_APPLICATION_ERROR_COMPONENT_NOT_LOADING', $option, JText::_('JLIB_APPLICATION_ERROR_COMPONENT_NOT_FOUND')), JLog::WARNING, 'jerror');

This comment has been minimized.

Show comment Hide comment
@eddieajau

eddieajau Nov 28, 2012

Contributor

I have a feeling the length of this line is just over the limit.

@eddieajau

eddieajau Nov 28, 2012

Contributor

I have a feeling the length of this line is just over the limit.

@okonomiyaki3000

This comment has been minimized.

Show comment Hide comment
@okonomiyaki3000

okonomiyaki3000 Nov 29, 2012

Contributor

Is there a rule about that? phpcs didn't complain.

Contributor

okonomiyaki3000 commented Nov 29, 2012

Is there a rule about that? phpcs didn't complain.

@elkuku

This comment has been minimized.

Show comment Hide comment
@elkuku

elkuku Nov 29, 2012

Member

It should generate a warning if the line contains more than 150 chars.

Maybe we should change that to error ?

Member

elkuku commented Nov 29, 2012

It should generate a warning if the line contains more than 150 chars.

Maybe we should change that to error ?

@eddieajau

This comment has been minimized.

Show comment Hide comment
@eddieajau

eddieajau Nov 29, 2012

Contributor

My personal rule-of-thumb is 120 characters (which gives me a bugger between 120 and 150 if I really need it). The reason is that 120 chars fits really well in the github code windows.

Contributor

eddieajau commented Nov 29, 2012

My personal rule-of-thumb is 120 characters (which gives me a bugger between 120 and 150 if I really need it). The reason is that 120 chars fits really well in the github code windows.

@okonomiyaki3000

This comment has been minimized.

Show comment Hide comment
@okonomiyaki3000

okonomiyaki3000 Nov 29, 2012

Contributor

I hope you mean buffer. Anyway, I didn't get a warning but the line was surely over the 150 character limit. I've broken it up into two.

Contributor

okonomiyaki3000 commented Nov 29, 2012

I hope you mean buffer. Anyway, I didn't get a warning but the line was surely over the 150 character limit. I've broken it up into two.

eddieajau added a commit that referenced this pull request Nov 29, 2012

Merge pull request #1720 from okonomiyaki3000/JComponentHelper-sprint…
…f-too-few-arguments

sprintf complains unless the correct number of arguments are passed

@eddieajau eddieajau merged commit ec2677e into joomla:staging Nov 29, 2012

@eddieajau

This comment has been minimized.

Show comment Hide comment
@eddieajau

eddieajau Nov 29, 2012

Contributor

Oops (blush). Merged. Thanks.

Contributor

eddieajau commented Nov 29, 2012

Oops (blush). Merged. Thanks.

@elinw

This comment has been minimized.

Show comment Hide comment
@elinw

elinw Jan 23, 2013

Contributor

I wanted to mention about this that the removal of that $error was also done during thee 12.2 to 12.3 period (#1572), but clearly without being positive what the implications are. And not to be a harpie about testing but ... we should not be doing things like that without serious unit tests in place especially in the legacy folder where applications are particularly counting on stability.

Contributor

elinw commented Jan 23, 2013

I wanted to mention about this that the removal of that $error was also done during thee 12.2 to 12.3 period (#1572), but clearly without being positive what the implications are. And not to be a harpie about testing but ... we should not be doing things like that without serious unit tests in place especially in the legacy folder where applications are particularly counting on stability.

@AmyStephen

This comment has been minimized.

Show comment Hide comment
@AmyStephen

AmyStephen Jan 23, 2013

Contributor

What is unfortunate is that everyone missed @ianmacl 's message at the time. Looks like he saw this right away.

But, in all fairness, the application was better off after the patch. It fixed one of two problems. Removing the second undefined variable basically turned in one warning for another. Still, +1 in the overall count. ;-)

Contributor

AmyStephen commented Jan 23, 2013

What is unfortunate is that everyone missed @ianmacl 's message at the time. Looks like he saw this right away.

But, in all fairness, the application was better off after the patch. It fixed one of two problems. Removing the second undefined variable basically turned in one warning for another. Still, +1 in the overall count. ;-)

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