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

Better installation errors #2 #6002

Closed
wants to merge 9 commits into from
Closed

Conversation

davdebcom
Copy link
Contributor

@davdebcom davdebcom changed the title Update installation error to display source and destination folders #2 Better installation errors #2 Feb 6, 2015
Corrected: Concat operator must be followed by one space
@wilsonge
Copy link
Contributor

wilsonge commented Feb 8, 2015

Hi,
Thanks for introducing this improvement to the CMS!

Please use a language string rather than hardcoding strings so that they are translatable. Also as this will change language strings it will have to wait until after 3.4 release (as we are currently in a language freeze)

@brianteeman
Copy link
Contributor

Is it not possible that this could be committed as a special case with the hardcoded strings NOW and then updated to use jtext later?

@Bakual
Copy link
Contributor

Bakual commented Feb 8, 2015

Imho it's not urgent enough to make a special exception for that.

@sovainfo
Copy link
Contributor

sovainfo commented Feb 8, 2015

Suggest to use:
JText::sprintf('JLIB_FILESYSTEM_ERROR_WARNFS_ERR02' . ' : %s => %s' , $src, $dest)

Could be changed later if required

@davdebcom
Copy link
Contributor Author

@sovainfo Changed to match your suggestion

Ready to go?

@sovainfo
Copy link
Contributor

Removed the space before the comma to make travis happy, sorry. Made a PR against yours.

What about the other JText::_ calls?
Like line 145 JLIB_FILESYSTEM_ERROR_COPY_FAILED

@davdebcom
Copy link
Contributor Author

@sovainfo Merged into my patch, thanks!

@coolcat-creations
Copy link
Contributor

Hi Guys, how to test this ? thx


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

@wilsonge
Copy link
Contributor

From what I understand you need to install an extension when having not enough file permissions in the components (or some folder that's needed as described in the link in the description) to move files into the directory. Before the patch you'll just see a message "Copy file failed" and after you'll see a message stating what the to and from directory are.

@coolcat-creations
Copy link
Contributor

Thanks, - tested successfully - just the translations are missing
JLIB_FILESYSTEM_ERROR_WARNFS_ERR02 : /Applications/MAMP/tmp/php/phpLYx2kB => /Applications/MAMP/htdocs/joomla-cms-staging/tmp/com_patchtester.tar.bz2


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

@zero-24
Copy link
Contributor

zero-24 commented Mar 14, 2015

RTC based on testing. Thanks


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

@brianteeman brianteeman added the RTC This Pull Request is Ready To Commit label Mar 14, 2015
@@ -140,7 +140,7 @@ public static function copy($src, $dest, $path = null, $use_streams = false)
{
if (!@ copy($src, $dest))
{
JLog::add(JText::_('JLIB_FILESYSTEM_ERROR_COPY_FAILED'), JLog::WARNING, 'jerror');
JLog::add(JText::_('JLIB_FILESYSTEM_ERROR_COPY_FAILED' . ' : %s => %s', $src, $dest), JLog::WARNING, 'jerror');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not using JText::sprintf so it won't work as expected.

@phproberto
Copy link
Contributor

Still requires a fix.

@zero-24
Copy link
Contributor

zero-24 commented Mar 15, 2015

Moving back to Pending based on @phproberto's commet.


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

@brianteeman brianteeman removed the RTC This Pull Request is Ready To Commit label Mar 15, 2015
@sovainfo
Copy link
Contributor

@davdebcom created pr to add sprintf to line 143

@zero-24 zero-24 added the RTC This Pull Request is Ready To Commit label Apr 1, 2015
@infograf768
Copy link
Member

This does not work! : the constant is not translated.

the code should be:

diff --git a/libraries/joomla/filesystem/file.php b/libraries/joomla/filesystem/file.php
index cc14b9c..585be6a 100644
--- a/libraries/joomla/filesystem/file.php
+++ b/libraries/joomla/filesystem/file.php
@@ -141,5 +141,5 @@
                if (!@ copy($src, $dest))
                {
-                   JLog::add(JText::_('JLIB_FILESYSTEM_ERROR_COPY_FAILED'), JLog::WARNING, 'jerror');
+                   JLog::add(JText::sprintf(JText::_('JLIB_FILESYSTEM_ERROR_COPY_FAILED') . ' : %1$s => %2$s', $src, $dest), JLog::WARNING, 'jerror');

                    return false;
@@ -520,5 +520,5 @@
                else
                {
-                   JLog::add(JText::_('JLIB_FILESYSTEM_ERROR_WARNFS_ERR02'), JLog::WARNING, 'jerror');
+                   JLog::add(JText::sprintf(JText::_('JLIB_FILESYSTEM_ERROR_WARNFS_ERR02') . ' : %1$s => %2$s', $src, $dest), JLog::WARNING, 'jerror');
                }
            }
@@ -539,5 +539,5 @@
                else
                {
-                   JLog::add(JText::_('JLIB_FILESYSTEM_ERROR_WARNFS_ERR02'), JLog::WARNING, 'jerror');
+                   JLog::add(JText::sprintf(JText::_('JLIB_FILESYSTEM_ERROR_WARNFS_ERR02') . ' : %1$s => %2$s', $src, $dest), JLog::WARNING, 'jerror');
                }
            }
 $src, $dest), JLog::WARNING, 'jerror');

Evidently, as the variables and the hardcoding of => are not in the language string value, we will get weird results for a RTL Language:

screen shot 2015-04-02 at 10 52 15

If we want to also solve this we could do it this way:

if (JFactory::getLanguage()->isRTL())
{
JLog::add(JText::sprintf(JText::_('JLIB_FILESYSTEM_ERROR_WARNFS_ERR02') . ' : %2$s <= %1$s', $src, $dest), JLog::WARNING, 'jerror');
}
else
{
JLog::add(JText::sprintf(JText::_('JLIB_FILESYSTEM_ERROR_WARNFS_ERR02') . ' : %1$s => %2$s', $src, $dest), JLog::WARNING, 'jerror');
}

we would get:
screen shot 2015-04-02 at 11 08 21

@infograf768
Copy link
Member

PR wrong, setting back to pending until corrected.


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

@zero-24 zero-24 removed the RTC This Pull Request is Ready To Commit label Apr 2, 2015
@sovainfo
Copy link
Contributor

sovainfo commented Apr 2, 2015

Added PR fixing translation and ltr by changing the messages in the language files.

@infograf768
Copy link
Member

@sovainfo
I commented on your PR. I suggest you link it to this and explain there how to test.

@sovainfo
Copy link
Contributor

sovainfo commented Apr 2, 2015

Changed my PR according to instructions from @infograf768. @davdebcom needs to accept my PR if he agrees.

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

Successfully merging this pull request may close these issues.

None yet

10 participants