ryancramerdesign / ProcessWire

Our repository has moved to https://github.com/processwire – please head there for the latest version.
https://processwire.com
Other
727 stars 201 forks source link

Warning at updates #2031

Closed juergenweb closed 8 years ago

juergenweb commented 8 years ago

I use Tracy Debugger and after pressing the check for updates button in the backend I always get the following message (warning not an error):

sprintf(): Too few arguments in wire/core/Modules.php at line 4162

This is the code line which will be marked by Tracy Debugger: count($versionChanges)), count($versionChanges)) .

Best regards Jürgen

ryancramerdesign commented 8 years ago

What PW version? I've gone through this line in the current 3.x code base multiple times now, but it appears the sprintf() arguments are correct (though still waiting for my coffee to kick in).

juergenweb commented 8 years ago

I use the latest PW 3.0.33. It always happens if I click the update button to grab new versions for the installed modules. I have Tracy Toolbar enabled and it shows me always the warning. If I skip the warning the updates will not be fetched.

screenshot_22

ryancramerdesign commented 8 years ago

Are you sure that's 3.0.33? The line numbers in your screenshot aren't correct. This is what's on line 4162: https://github.com/ryancramerdesign/ProcessWire/blob/devns/wire/core/Modules.php#L4162

Unless Tracy is using some different way of mapping line numbers, It appears at least that your Modules.php file is an older version. However, I'm not sure that's the issue here, as the highlighted block of code is the same there as it is on the current source. All the braces line up correctly as far as I can tell.

@adrianbj any idea on this one, or am I missing an obvious code error here?

adrianbj commented 8 years ago

@ryancramerdesign - take a look at this blob's line 4162: https://github.com/ryancramerdesign/ProcessWire/blob/e4c8339635f7531b9b3b5725614e883532876c31/wire/core/Modules.php#L4162

That matches the block of code reported by Tracy.

juergenweb commented 8 years ago

I am sure that I use 3.0.33. I have updated the site with the updater module all the time and it shows me 3.0.33 as the version number in the backend. I have never updated it manually with FTP.

screenshot_1

I have also discovered this warning by installing and deinstalling of modules.

ryancramerdesign commented 8 years ago

@adrianbj It must be that I have added some code since then. But the non-matching line numbers is just the sideshow, as the error message coming from Tracy is the one I was wondering about. Tracy is pointing out an error message, but I don't see that there's an actual code error there. Maybe I'm missing something? :)

adrianbj commented 8 years ago

But the non-matching line numbers is just the sideshow

I completely agree - I was just clarifying your statement:

Unless Tracy is using some different way of mapping line numbers

Honestly I have never been much of a sprintf user. It almost seems like the sprintf is being processed before the _n which of course doesn't make any sense :) I can't reproduce the error at my end, so I don't think it's Tracy specific. If I could reproduce, I guess I would start by removing the _n so I am including hard values for all the sprintf arguments. I guess this suggestion is to you @juergenweb since neither of us can reproduce.

Could you perhaps try replacing that block with this?

        if(count($versionChanges)) {
            $this->message(
                sprintf('Detected %d module version change', 1) . 
                ' (' . $this->_('will be applied the next time each module is loaded') . '):' . 
                '<pre>' . implode("\n", $versionChanges) . '</pre>', 
                Notice::allowMarkup | Notice::debug);
        }

I know there is no logical reason that this should be any different, but sometimes you have to start somewhere :)

juergenweb commented 8 years ago

@adrianbj

The replacement works. The warning is gone!!

adrianbj commented 8 years ago

Ok great - now let's work backwards.

Could you add back in count($versionChanges) rather than the hardcoded "1" ?

I think that should be ok, but if it's not, could you find out the value of $versionChanges and also count($versionChanges)

I think the issue is likely with:

$this->_n('Detected %d module version change', 'Detected %d module version changes', count($versionChanges);

so it would also be good to know what the result is of that call on its own. I feel like that must be failing resulting in two "%d" which is why the argument count doesn't match.

LostKobrakai commented 8 years ago

@juergenweb You're using a language pack? probably the translation does have the additional %d or alike.

juergenweb commented 8 years ago

Maybe this could be the reason, because if I remove the %d from the string it works fine. Yes I use a language pack.

This works: sprintf($this->_n('Detected module version change', 'Detected module version changes', count($versionChanges))) .

This works not: sprintf($this->_n('Detected %d module version change', 'Detected %d module version changes', count($versionChanges))) .

LostKobrakai commented 8 years ago

Could you see what the translation is for that file?

adrianbj commented 8 years ago

Remember that the "%d" is needed - that is what sprintf replaces with the value of count($versionChanges)

I get the feeling that @LostKobrakai is correct with the language pack idea and it missing the "%d".

It's then a case of the _n function choosing the single vs plural version of that text so there is only one "%d" to be replaced.

juergenweb commented 8 years ago

The strings were not translated. Now I have translated both strings and it works now. screenshot_3

Conclusion: If I enter no translation or "=" in the translation field it outputs the warning. If I enter a translation which includes the "%d" it works.

isellsoap commented 8 years ago

@juergenweb This issue seems to be fixed, thus I close it. If you think the issue isn’t fixed yet or requires additional work to do, please consider reopening it.