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

Extend the message to explain what is happening #16246

Closed
wants to merge 12 commits into from

Conversation

zero-24
Copy link
Contributor

@zero-24 zero-24 commented May 24, 2017

Pull Request for Issue #16238

Summary of Changes

In some broken hosting envoirments both mime checker tools are not enabled. In that case show that message with more extended info.

Testing Instructions

  • Try this on a broken hosting.

  • confirm the extended message

  • review the text.

Expected result

Extended text explaining what is happening

Actual result

Just a general message which is not longer match the usage.

Documentation Changes Required

None

@mbabker
Copy link
Contributor

mbabker commented May 24, 2017

This needs to be crafted carefully. This message might be appropriate for the site owner who would be the primary POC regarding server configurations, but for a general user of the site this is too much information (they shouldn't be seeing a message saying "this server is incorrectly configured" basically, and that's what this message says).

@zero-24
Copy link
Contributor Author

zero-24 commented May 24, 2017

I'm happy to accept a better suggestion. Maybe we show a different message in frontend and backend?

@brianteeman
Copy link
Contributor

brianteeman commented May 24, 2017

yes if possible to have a different message on the frontend that would be better

also please change him to them to make it gender neutral.

I will wait to hear if the message can be split before commenting on the text itself

@mbabker
Copy link
Contributor

mbabker commented May 24, 2017

The error message needs to be somewhat vague as to the cause of the issue still (the user just needs to know the upload failed, not everyone needs to know the technical reasoning for the failure), but we should be making it possible for site owners to get more information about why the issue happened. Long and short, we need to start making more liberal use of our logging system and stop expecting that the only way to get information about an error state is through messages displayed to the user in the alert container.

So a message displayed in the UI should basically say not much more than "could not upload file because the MIME type could not be determined". A supplemental JLog::add() call should be used to log additional details (upload of X file failed because the MIME type could not be determined).

@zero-24
Copy link
Contributor Author

zero-24 commented May 24, 2017

I will wait to hear if the message can be split

Yes it can as we have different language files for backend and frontend.

@brianteeman
Copy link
Contributor

brianteeman commented May 24, 2017

first attempt

Frontend

+JLIB_MEDIA_ERROR_WARNINVALID_MIME="We were unable to upload this file."

Backend

+JLIB_MEDIA_ERROR_WARNINVALID_MIME="We could not detect the mime type for this file. For security purposes this file has not been uploaded. Please contact your host to check that the mime_content_type or the fileinfo extension is enabled in php."

@zero-24
Copy link
Contributor Author

zero-24 commented May 24, 2017

done with the last commit.

@Quy
Copy link
Contributor

Quy commented May 24, 2017

I prefer not to not use We were and We could since we don't use We in the other strings.

How about?

Unable to upload this file.

Unable to detect mime type for this file. For security purposes have not uploaded this file. Please contact your host and to check that mime_content_type or the fileinfo extension is enabled in PHP.

@brianteeman
Copy link
Contributor

i was trying to be more friendly - but if not then

Unable to upload this file.

Unable to detect the mime type for this file. For security purposes this file has not been uploaded . Please contact your host and check that the mime_content_type or the fileinfo extension is enabled in PHP.

@zero-24
Copy link
Contributor Author

zero-24 commented May 24, 2017

pushed thanks

@@ -670,7 +670,8 @@ JLIB_MEDIA_ERROR_WARNFILETOOLARGE="This file is too large to upload."
JLIB_MEDIA_ERROR_WARNFILETYPE="This file type is not supported."
JLIB_MEDIA_ERROR_WARNIEXSS="Possible IE XSS Attack found."
JLIB_MEDIA_ERROR_WARNINVALID_IMG="Not a valid image."
JLIB_MEDIA_ERROR_WARNINVALID_MIME="Invalid mime type detected."
; Do not sync that line with the frontend! This is expected to be different from the frontend string.
Copy link
Contributor

Choose a reason for hiding this comment

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

the next line instead of that line
please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@Quy
Copy link
Contributor

Quy commented May 24, 2017

i was trying to be more friendly - but if not then

I totally understand. Just going for consistency sake.

@@ -670,7 +670,8 @@ JLIB_MEDIA_ERROR_WARNFILETOOLARGE="This file is too large to upload."
JLIB_MEDIA_ERROR_WARNFILETYPE="This file type is not supported."
JLIB_MEDIA_ERROR_WARNIEXSS="Possible IE XSS Attack found."
JLIB_MEDIA_ERROR_WARNINVALID_IMG="Not a valid image."
JLIB_MEDIA_ERROR_WARNINVALID_MIME="Invalid mime type detected."
; Do not sync that line with the frontend! This is expected to be different from the frontend string.
JLIB_MEDIA_ERROR_WARNINVALID_MIME="Unable to detect the mime type for this file. For security purposes this file has not been uploaded . Please contact your host and check that the mime_content_type or the fileinfo extension is enabled in PHP."
Copy link
Contributor

Choose a reason for hiding this comment

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

extra space after uploaded

sorry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed. np.

@Quy
Copy link
Contributor

Quy commented May 24, 2017

How about removing this file from the 1st sentence since it is mentioned (redundant) in the second sentence?

Unable to detect the mime type. For security purposes this file has not been uploaded. Please contact your host and check that the mime_content_type or the fileinfo extension is enabled in PHP.

@brianteeman
Copy link
Contributor

Fine by me

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on 11a71e9

on review


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

@brianteeman
Copy link
Contributor

just had a thought and running for a train so cant test BUT is this message also displayed when you are trying to upload an file that is not in the allowed mime types that are saved in the com-media options? If so then this new message is not correct

@zero-24
Copy link
Contributor Author

zero-24 commented May 25, 2017

No than we have a differnent message that also include the detected mime type.
This message is only displayed if we could not detect a mimetype and this means that both methos are not enabled.

@brianteeman
Copy link
Contributor

ok all good then

@Quy
Copy link
Contributor

Quy commented May 25, 2017

I have tested this item ✅ successfully on 11a71e9


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

@ghost
Copy link

ghost commented May 25, 2017

RTC after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 25, 2017
@mbabker
Copy link
Contributor

mbabker commented May 25, 2017

I am still going to be insistent that the UI does not use an error message that says "you need X PHP extension/function enabled". That comment is only beneficial if the only user who may see it is the site owner or can manage their server configuration.

@brianteeman
Copy link
Contributor

@mbabker are the people who can see that message different to the people who can see the php update message?

@mbabker
Copy link
Contributor

mbabker commented May 25, 2017

Depending on how you have the plugin configured or your site's ACL, yes, it could be a different group.

@rdeutz rdeutz added this to the Joomla 3.7.3 milestone May 25, 2017
@rdeutz rdeutz removed the RTC This Pull Request is Ready To Commit label May 25, 2017
@joomla-cms-bot joomla-cms-bot removed this from the Joomla 3.7.3 milestone May 25, 2017
@rdeutz
Copy link
Contributor

rdeutz commented May 25, 2017

seems to me not really discussed how it should be, state set to "needs review"


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

@zero-24
Copy link
Contributor Author

zero-24 commented Aug 13, 2017

I'm closing here as it looks like we can not find a consense here. Thanks.

@zero-24 zero-24 closed this Aug 13, 2017
@zero-24 zero-24 deleted the better_mime_error_message branch August 13, 2017 14:12
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

6 participants