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

Add source & dest to copy/move errors #6644

Merged
merged 6 commits into from
Apr 15, 2016
Merged

Conversation

sovainfo
Copy link
Contributor

@sovainfo sovainfo commented Apr 3, 2015

Still fighting with github to make it do what we want. Doesn't allow me to add changes to this pr.

@wilsonge
Copy link
Contributor

wilsonge commented Apr 3, 2015

If we aren't using the old strings anymore can we deprecate them please? Otherwise come Joomla 4.0 it's going to be horrendus to work out what strings we are and aren't using. Plus it makes it harder for developers to know what strings to do

@@ -260,6 +261,7 @@ JLIB_FILESYSTEM_ERROR_UPLOAD="JFile: :upload: %s"
JLIB_FILESYSTEM_ERROR_WARNFS_ERR01="Warning: Failed to change file permissions!"
JLIB_FILESYSTEM_ERROR_WARNFS_ERR02="Warning: Failed to move file!"
JLIB_FILESYSTEM_ERROR_WARNFS_ERR03="Warning: File %s not uploaded for security reasons!"
JLIB_FILESYSTEM_ERROR_WARNFS_ERR04="Warning: Failed to move file: %s => %s"
Copy link
Contributor

Choose a reason for hiding this comment

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

@sovainfo can you use here also %1s => %2s 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zero-24 Thanks

Added sequence numbers for the parameters as pointed out by @zero-24
@sovainfo
Copy link
Contributor Author

sovainfo commented Apr 3, 2015

Where is the documentation on deprecating messages?

@brianteeman
Copy link
Contributor

There is no documentation for deprecating language strings. I did propose something but this was rejected.

@wilsonge How do you propose to deprecate the strings. The problem is that the stupid ancient way that TT are given strings to translate will not be able to show if a string is deprecated


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

@sovainfo
Copy link
Contributor Author

sovainfo commented Apr 4, 2015

Considering there is no documentation on deprecating language string, this PR should be set to RTC.
See #6001 & #6002

With special thanks to @davdebcom, @designbengel and @infograf768

@brianteeman
Copy link
Contributor

Test instructions from @wilsonge 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.


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

@mikeveeckmans
Copy link

I have tested this item ✅ successfully on bd27a90

TEST OK
(J3.5.1 , PHP 7)
made tmp folder locked and unwriteable.

screen shot 2016-04-07 at 04 42 59


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

@infograf768
Copy link
Member

Not sure I understand the messages:

Warning
Warning: Failed to move file: /Applications/MAMP/tmp/php/phptyMAxf => /Applications/MAMP/htdocs/trunkgitnew/tmp/jostag_plugin_for_2.5.zip
Error
Archive does not exist
Unable to find install package

what is php/phptyMAxf ?

Should not the message says
Impossible to move the jostag_plugin_for_2.5.zip file to the /Applications/MAMP/htdocs/trunkgitnew/tmp/ folder ?

Also, I would use to instead of => as it would be easier for rtl translations.

@brianteeman
Copy link
Contributor

Valid point about using to

On 7 April 2016 at 11:22, infograf768 notifications@github.com wrote:

Not sure I understand the messages:

Warning
Warning: Failed to move file: /Applications/MAMP/tmp/php/phptyMAxf => /Applications/MAMP/htdocs/trunkgitnew/tmp/jostag_plugin_for_2.5.zip
Error
Archive does not exist
Unable to find install package

what is php/phptyMAxf ?

Should not the message says
Impossible to move the jostag_plugin_for_2.5.zip file to the
/Applications/MAMP/htdocs/trunkgitnew/tmp/ folder ?

Also, I would use to instead of => as it would be easier for rtl
translations.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#6644 (comment)

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@mbabker
Copy link
Contributor

mbabker commented Apr 7, 2016

@infograf768 That path is the temporary path & name that whatever file you tried uploading gets sent to by PHP before the underlying code ultimately moves it into place. JFile doesn't know what the name of the file you tried uploading, only what the source path it's located at is, so that's all the message can display when rendered from there. To get a message like what you're looking for, it would need to be rendered by the method processing the upload as that will have access to the data extracted from the $_FILES superglobal (which contains that name).

@joomla-cms-bot
Copy link

This PR has received new commits.

CC: @mikeveeckmans


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

@joomla-cms-bot joomla-cms-bot added the Language Change This is for Translators label Apr 8, 2016
@joomla-cms-bot
Copy link

This PR has received new commits.

CC: @mikeveeckmans


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

@brianteeman
Copy link
Contributor

Why create a new string JLIB_FILESYSTEM_ERROR_WARNFS_ERR04 instead of updating JLIB_FILESYSTEM_ERROR_WARNFS_ERR02


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

@infograf768
Copy link
Member

@mbabker
I understand what you say. In that case I think that it is useless to display the source in the message as it is not understandable.
Why not simply use
Warning: Failed to move file to /Applications/MAMP/htdocs/trunkgitnew/tmp/jostag_plugin_for_2.5.zip

Btw, Warning: may be useless as it is stated in the Header of the message

@mbabker
Copy link
Contributor

mbabker commented Apr 9, 2016

Source is only "not understandable" if you're moving something from a temporary location with a temporary file name. JFile doesn't know whether it's been told to move something from the temporary location or told to move the index.php file to new.php. So basically if you say hide it because some paths may look like gibberish then you have to not show the source path for all cases.

@sovainfo
Copy link
Contributor Author

sovainfo commented Apr 9, 2016

This PR corrects the mistake by core code of being too abstract. When something goes wrong it is not good enough to report something went wrong. You need to provide sufficient details in order to investigate the cause of whatever went wrong. Would be nice if Joomla was made so robust it would tell the cause, can't wait for that! In the mean time it would be helpful when Joomla provides sufficient details so you can investigate what the cause is. The fact core code is wrongly using the original messages doesn't mean any code using it is wrong. So, the core code must be changed to use a different message. Haven't found the new message, so it needed to be created. Obviously, any code making the same mistake can be corrected the same way. Correct usage of the message doesn't need any change!

@coolcat-creations
Copy link
Contributor

I have tested this item ✅ successfully on 1755915


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

@mikeveeckmans
Copy link

I have tested this item ✅ successfully on 1755915

test OK


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

@brianteeman
Copy link
Contributor

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Apr 15, 2016
@brianteeman brianteeman added this to the Joomla 3.5.2 milestone Apr 15, 2016
@rdeutz rdeutz merged commit 6a9117f into joomla:staging Apr 15, 2016
@rdeutz rdeutz modified the milestones: Joomla 3.5.2, Joomla! 3.6.0 May 1, 2016
@brianteeman brianteeman removed the RTC This Pull Request is Ready To Commit label May 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants