Is it really a good idea to ever suppress errors

The thing is: i have a PR (#12671) for this method to be used across the core (including frontend) and as it is if somehow the language xml is not readble it will trow an exception. Before it wouldn't (i think), so to preserve the same behaviour i made this PR.

Yes catch the exception and avoid the meaningless fatal error, but issue no message at all ?

why not replace the fatal meaningless error with a good warning message if JApplication is backend, instead of ignoring it silently ?

It's consistent with other checks in that part of the code, seems fine to me. At most I'd suggest a log message but I don't see the need to display a message in the UI regardless of client.

ok. added log messages

humm ... this way i also get warning messages (?)

Logging to the jerror category causes this. Log to any other category and it won't automagically get added to the message queue.

Logging to the jerror category causes this. Log to any other category and it won't automagically get added to the message queue.

hum ... are there other categories besides database, databasequery, database-error, deprecated and jerror?

You can use whatever you want for the category. Those are just the
standard values we use in core.

On Monday, November 7, 2016, andrepereiradasilva notifications@github.com
wrote:

Logging to the jerror category causes this. Log to any other category and
it won't automagically get added to the message queue.

hum ... are there other categories besides database, databasequery,
database-error, deprecated and jerror?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#12783 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoTxKHJaIvH9U9opJXlpFJEmUb0ZHks5q7zdvgaJpZM4Kqh_F
.

oh ok, learning every day ?

I agree with @ggppdk
I would prefer to get a good old Error message in back-end. That would be real useful for admins.

the thing is the old message was an individual check in com_languages installed model that was making an hardcoded db query to fetch the installed languages.

That individual check was removed since the languages are now fetched from JLanguageHelper::getInstalledLanguages.

I ask you to see the global perspective here, not a particular place where the warning message appear (by the way it was a PR of mine that putted there before ? ).

The installed languages currently are checked (db extension, file, metadata) is some places and others not.
The end goal of moving all this to JLanguageHelper::getInstalledLanguages is to avoid code repetition and with that: easier code mantain, perfomance/memory improvings and especcialy correct/avoid errors and unexpected behaviours.

Besides this PR and the one already merged (#12641) i have made already another PR (#12671) to use always this new method, so we can't send the warning messages (neither trow exceptions) everywhere the instralled languages are fetched with issues, instead we now log those issues to the debug console in a new language category.

In summary, you can still see the warning messages if you enable debug and system debug plugin and go to log messages tab.

Note: I also plan to do something similiar with the content languages in the future.

I have tested this item ✅ successfully on 2a27a73

@alikon can you test this one too?

could we get another test here.
Without this and since #12671 was already merged now we have exceptions in staging if you have a language installed without the language files.

Tested fine. However I wasn't able to get the log message appear. Personally I would prefer a warning message as well, since there is something wrong in the system for sure if that case happens.

Also I'm not sure why you try/catch the parseXMLInstallFile call. That one returns a array|boolean and doesn't throw anything from what I see.

Also I'm not sure why you try/catch the parseXMLInstallFile call. That one returns a array|boolean and doesn't throw anything from what I see.

simplexml_load_file() instantiates a new SimpleXMLElement object and its constructor can throw an Exception.

@Bakual can you mark as tested with success?

BTW i choose to not trow a warning to not send a warning in the frontend on upgrade to 3.7.0 in case users have a missing language file. A log message is enough IMO.

Tested fine. However I wasn't able to get the log message appear. Personally I would prefer a warning message as well, since there is something wrong in the system for sure if that case happens.

This is a high level library class. So this shouldn't be directly attached to the application classes or the message queue. Next, this error isn't appropriate for the frontend if this API ever gets used there as it would include an information disclosure because file paths are being used, so if you wanted to enqueue messages, now you'd have to have application specific behaviors in a high level library class (even worse, I'm still annoyed by that in the Redis cache adapter). So that means letting the error bubble and the callers need to catch errors, and lets face it, people don't do error handling in Joomla (even the error handling in core is barely a step above bad).

I have tested this item ✅ successfully on 2a27a73

RTC

This is a high level library class. So this shouldn't be directly attached to the application classes or the message queue.

Yes sure. Throwing the exception in JLanguage is fine. Catching that and enqueueing a message in the JLanguageHelper however is no issue as that class has several couplings to the application.
As for frontend, the helper class can (and does) distinguish it. So the warning could only be shown in backend. And obviously I wouldn't show the filepath. It isn't needed anyway for a helpful message.
See my PR (which I did without knowing of this one): #12921

can you mark as tested with success?
@andrepereiradasilva No, because I wasn't able to see the log message, disagree with only logging and had another question. So it's not a successfull (but also not really failed) test for me.

if you don't see a log message is probably because you have the cache enabled (the cache issue will be solved with #12906) or you didn't enable the global config debug and debug plugin to show those log messages.

Just because we already have bad architecture doesn't mean we should add more.

Catching that and enqueueing a message in the JLanguageHelper however is no issue as that class has several couplings to the application.

No it doesn't. It actually has one and that's for the install app. The rest of the helper class is (correctly) application ignorant. This isn't to be confused with the client ID parameter in the getInstalledLanguages() which isn't a coupling to the application; it's a parameter to filter data, not something that requires use of the application object.

So the warning could only be shown in backend.

Anything in the libraries folder which is making decisions to enqueue a message based on the active application has a major code smell. Message enqueuing should not be so liberally used everywhere as that creates dependencies to the application object, and inherently JFactory, and if anyone ever actually gave the test suite the proper attention you'd realize what kind of nightmare that creates because of how much of the global state you have to mock just to be able to run a test condition.

It [file path] isn't needed anyway for a helpful message.

For debugging missing file errors, it actually is pretty helpful. Unless you're assuming the message can be written in a way that someone debugging it can immediately decipher what the cause of the issue was. Your PR honestly just adds another vague error message to be displayed.

I wasn't able to see the log message

Ensure you have a logger set up catching the "language" category. Since Joomla by default doesn't turn on any loggers except to proxy the "jerror" category to the message queue it won't be logged without turning on additional tools.

Found why the log didn't work. When testing this PR with the patchtester, the log wasn't created for some reason. There were also other changes than the ones from this PR. That's due to the way the Patchtester works (replacing files instead of merging changes).

When merging directly from you branch, the log appeared when the debug plugin had "Log Almost Everything" enabled as expected. Btw the global debug setting doesn't play a role here.

Still don't like that this is hidden in a log which nobody will see. But if I'm the only one here, feel free to merge it.

You are not the only one, but, whatever... ?