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

Handle DB errors in categories search plugin #7073

Closed
wants to merge 7 commits into from
Closed

Handle DB errors in categories search plugin #7073

wants to merge 7 commits into from

Conversation

alikon
Copy link
Contributor

@alikon alikon commented May 30, 2015

added the generic message string "There has been an error" as suggested in #6710

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-staging labels May 30, 2015
@alikon
Copy link
Contributor Author

alikon commented May 30, 2015

pr6710

@brianteeman
Copy link
Contributor

Sorry can you add a full stop at the end

There has been an error.

On 30 May 2015 at 18:00, Nicola Galgano notifications@github.com wrote:

[image: pr6710]
https://cloud.githubusercontent.com/assets/181681/7898100/a0e37dac-06f5-11e5-9bf9-5d5494dc4a5b.png


Reply to this email directly or view it on GitHub
#7073 (comment).

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

@wilsonge
Copy link
Contributor

You have a binary patch file for some reason :P sorry!

@alikon
Copy link
Contributor Author

alikon commented May 30, 2015

no, only my mistake

@infograf768
Copy link
Member

Please add
JLIB_DATABASE_GENERIC_SQL_ERROR="There has been an error."
also in en-GB.lib_joomla.ini both site and admin

as the one in en-GB.ini is a fall back

; if there is an error connecting database before initialisation, en-GB.lib_joomla.ini can't be loaded
; we therefore have to load the strings from en-GB.ini

removed Whitespace found at end of line 181
as requested #7073 (comment)
the side one
as requested #7073 (comment)
the admin one
@alikon
Copy link
Contributor Author

alikon commented Jun 5, 2015

@infograf768 strings added
we can close #6710 in favour of this one that is more complete

@Bakual
Copy link
Contributor

Bakual commented Jun 7, 2015

Do we need this string also in administrator/language/en-GB/en-GB.ini ? Or is only the frontend used as fallback?

I wonder if we cna extend the error message a tiny bit. Currently there is absoluetly no value in it. You could as well show nothing because the user can probably figure out himself that there was some error.
I would at leats point him to the database as the suspect. So something like "There was an error related to the database." Better wording needed for sure, but make the error message at least contain some information.
@brianteeman @wilsonge What do you think?

@wilsonge
Copy link
Contributor

wilsonge commented Jun 7, 2015

So don't forget the full message is going to be stored in the logs. This is a message targeted at the frontend user who wants to see content and doesn't care where the error message comes from. That's how me and Brian came up with this string

@Bakual
Copy link
Contributor

Bakual commented Jun 7, 2015

Yeah, I'm aware it's going to be logged.
But to the user seeing this message, it's useless. You could as well use the existing one JERROR_AN_ERROR_HAS_OCCURRED="An error has occurred."

I would even suggest to extend the message and include something like the existing one for JLayout (?):
JERROR_LAYOUT_PLEASE_CONTACT_THE_SYSTEM_ADMINISTRATOR="If difficulties persist, please contact the System Administrator of this site and report the error below."

So I would even use something like:
JLIB_DATABASE_GENERIC_SQL_ERROR="There has been an error in the database query. If difficulties persist, please contact the System Administrator of this site and report the error below."

Don't show the actual error, but give the user (who may be the admin!) a clue what happend.

@Bakual
Copy link
Contributor

Bakual commented Jun 7, 2015

We could even tell in the message that the actual error was logged. That would be most helpful to the admin.

@wilsonge
Copy link
Contributor

wilsonge commented Jun 7, 2015

I agree having a "report the error below" might help - but then we need to have something significant the backend admin understands it but enough that the frontend user doesn't understand it...

@Bakual
Copy link
Contributor

Bakual commented Jun 7, 2015

the frontend user doesn't understand it...

He is allowed to understand. That's not a problem at all as long as no sensible data is shown.

@alikon
Copy link
Contributor Author

alikon commented Jun 8, 2015

if i understand well, @Bakual you want a little bit more informative msg
the msg should be something like this
"There has been an error and has been reported on the LOG file."
if i'm correct can someone correct/review the new msg ?

@brianteeman
Copy link
Contributor

Sorry but there is zero value telling a guest that something has been
logged when they can't see the log.
On 8 Jun 2015 14:18, "Nicola Galgano" notifications@github.com wrote:

if i understand well, @Bakual https://github.com/bakual you want a
little bit more informative msg
the msg should be something like this
"There has been an error and has been reported on the LOG file."
if i'm correct can someone correct/review the new msg ?


Reply to this email directly or view it on GitHub
#7073 (comment).

@Bakual
Copy link
Contributor

Bakual commented Jun 8, 2015

Sorry but there is zero value telling a guest that something has been
logged when they can't see the log.

There is even less value in showing "There was an error" when the guest already can see that by looking at the page.
I don't care much. My point is either create a new string which also has some value in the message, or use the already existing JERROR_AN_ERROR_HAS_OCCURRED="An error has occurred.".
Personally I would prefer the first option but I can sure also live with the second if you all feel it's to much detail.

@alikon
Copy link
Contributor Author

alikon commented Jun 9, 2015

ok, i'll wait till a final decision will be taken, where all agree...

@Bakual
Copy link
Contributor

Bakual commented Jun 9, 2015

Just do what @wilsonge says. He is in charge 😄

@anibalsanchez
Copy link
Contributor

Just in case, it works OK


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

@zero-24 zero-24 added this to the Joomla! 3.4.2 milestone Jun 15, 2015
@Bakual Bakual closed this in 06b38e2 Jun 16, 2015
@Bakual
Copy link
Contributor

Bakual commented Jun 16, 2015

After having a chat with @wilsonge we settled for JERROR_AN_ERROR_HAS_OCCURRED="An error has occurred." for now. We can always use a different string if needed in future.
I merged the plugins PRs, removed the new language string and changed the string to the existing one.

@alikon
Copy link
Contributor Author

alikon commented Jun 18, 2015

thx for take care of these

@wilsonge
Copy link
Contributor

No THANKYOU for going through the CMS and finding all these places where we weren't catching exceptions!

@alikon
Copy link
Contributor Author

alikon commented Jun 18, 2015

It's a pleasure to work with you folks, I like costructive critic to mY dirty fixes...

johanjanssens pushed a commit to joomlatools/joomla-fork that referenced this pull request Jun 19, 2015
@alikon alikon deleted the handledb branch March 30, 2017 11:37
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

8 participants