search

joomla / joomla-cms

Home of the Joomla! Content Management System
https://www.joomla.org
GNU General Public License v2.0
4.7k stars 3.63k forks source link

Inconsistencies and illogicalities in enqueueMessage #29926

Open rogercreagh opened 4 years ago

rogercreagh commented 4 years ago

Steps to reproduce the issue

$app = JApplicationCms::getInstance($location);
$app->enqueueMessage($message, $messagetype);

(I understand this is the replacement for the deprecated Jfactory version - it is a bit less useful as you have to specify whether you are in 'site' or 'administrator' for the location, so code is not easily portable between front and back end)

with the following options for $messagetype '' (empty string) 'message' 'Message' 'success' 'Success' 'info' 'Info' 'information' 'Information' 'warning' Warning' 'error 'Error' 'anythingelse'

Expected result

'', 'message', 'Message' = colour green, heading 'Message' 'success','Success' = colour green, heading 'Success'
'info', 'Info' = colour blue, heading 'Information' 'information, 'Information' = colour blue, heading 'Information' 'warning',Warning' = colour yellow, heading 'Warning' 'error','Error' = colour red, heading 'Error' 'anythingelse' = colour white/lightgrey, heading 'Anythingelse'

Actual result

'', 'message', 'Message' = colour green, heading 'Message' OK 'success','Success' = colour green, heading 'success' BAD - inconsistent capitalisation 'info', 'Info' = colour blue, heading 'Information' OK 'information, 'Information' = colour yellow, heading 'information' BAD - forced use of abbreviation 'warning',Warning' = colour yellow, heading 'Warning' OK 'error','Error' = colour red, heading 'Error' OK 'anythingelse' = colour yellow, heading 'anythingelse' BAD - not logical

'Success' is typically a more useful heading than the generic 'Message' for the green result. It should be capitalised. All the other headings reflect the result type (error, info, warning), success should do the same. Message should be kept as a more generic alternative where Info is not appropriate, but arguably should have a different colour (perhaps cyan)

'Information' should be an alternative to the abbreviation 'Info'. It may seem trivial but it is important to have the option not to use the abbreviation. 'Information' or 'information' should produce the same colour as 'info' and be consistent with success/message.

'anythingelse' would be a useful extension for messages that could benefit from a more informative one word title. The choice of yellow as the default colour is not always appropriate. The word provided as the type should be shown capitalised to be consistent with the other types (or at worst simply reflect the capitalisation in the $msgtype string rather than being forced to lower case). The colour could be white/lightgrey with a grey border, or cyan to match the proposal for the generic Message option.

System information (as much as possible)

J3.9.19

Additional comments

This needs to be carried forward into J4. As far as I can tell the same inconsistencies and illogicality in the handling of 'success'/'Success' have been perpetrated (there doesn't seem to be any documentation on this), and the proposal for Information and Anythingelse would be a useful enhancement.

ReLater commented 4 years ago

I can't confirm all your finds (e.g. Success is capitalized in my case) but I agree that in case of e.g. anythingelse some flexibility has been gone in J4. EDIT: And I think that it's a JS bug.

Testing:

$app->enqueueMessage('Testing type: anythingelse.', 'anythingelse ');

1) Create a langugae string ANYTHINGELSE="My custom Headline for Something". Headline OK. J3/J4.

2) In Joomla 3 we had the possibility to CSS override class alert alert-anythingelse.

03-07-_2020_15-43-16

In Joomla 4 we have not.

Joomla 4 ignores the custom $messagetype and always falls back to messagetype info. Therefore we cannot style it like we want.

03-07-_2020_15-51-01

That's an JS issue because the PHP-JLayout joomla.system.message builds the HTML correctly.

<joomla-alert type="anythingelse" dismiss="true">

ReLater commented 4 years ago

See also https://github.com/joomla-projects/custom-elements/issues/142 please.

rogercreagh commented 4 years ago

See also joomla-projects/custom-elements#142 please.

👍
So it is alert.js - I was wondering where it was done but couldn't find it. Thanks.

Just to add to the inconsistencies I see that msgtype 'danger' or 'Danger' also produces a red background but without capitalising 'danger'. Same issue as with success. 'Danger' might be a useful alternative to 'Error' in some instances so this should be capitalised.

In summary the changes required are:

  1. Initial capitalise 'success' as the heading for msgtype success
  2. Initial capitalise 'danger' as the heading for msgtype danger
  3. Change the background colour for msgtype blank or 'message' to cyan (to distinguish it from Success)
  4. Allow at least a one-word msgtype [anythingelse] (not literally 'anythingelse' 😉 ) which would use the word presented as msgtype for the heading and the alert- class name allowing custom css to set the required background colour. Ideally the capitalisation of the word would be as presented in the $msgtype variable, but since this would require moving the strtolower or lcase to further down the processing chain it might be easier to restrict [anythingelse] to a single word and just initial capitalise the heading as with the others in the first instance.
  5. Allow msgtype 'information' as an alias for 'info' with the heading presented as 'Information'
brianteeman commented 4 years ago

The strings are NOT being capitalised they are being tanslated

rogercreagh commented 4 years ago

I can't confirm all your finds (e.g. Success is capitalized in my case)

image from Joomla 3.9.19 (test install, no language or other overrides) viewed with Brave (chrome) browser. Same result whether msgtype is 'success' or 'Success'

rogercreagh commented 4 years ago

The strings are NOT being capitalised they are being tanslated

Ahha - I see (why is searching in the language overrides so vague - searching for 'message' as a value brings up everything containing message as part of a word in the whole string. Searching there needs improving ... separate issue). There is a default language string for MESSAGE but not for SUCCESS So points 1 & 2 can be met by introducing default language constants for SUCCESS="Success" and DANGER="Danger". This would seem a sensible minimal PR to start with (but I can't create a PR - perhaps someone competent could do it.). Also INFORMATION="Information", would partially meet point 5 although the colour would still be wrong, it needs to be mapped to the same alert type as 'info'

ReLater commented 4 years ago

from Joomla 3.9.19

Same result whether msgtype is 'success' or 'Success'

Maybe. I have deleted already my test environments. I would say then there is missing a language string like

SUCCESS="Success"

See https://github.com/joomla/joomla-cms/issues/29926#issuecomment-653598056

Automatic capitilising would be wrong for some languages.

Also INFORMATION="Information", would partially meet point 5 although the colour would still be wrong, it needs to be mapped to the same alert type as 'info'

I would say "no". Nothing should be mapped to another message type than the one used in enqueueMessage.

BUT you can map custom (and core) types to whatever you want in https://github.com/joomla/joomla-cms/blob/4.0-dev/layouts/joomla/system/message.php#L27

J4 only.

BUT only if alert.js is adapted accordingly.. That's the frustrating bottleneck here.

brianteeman commented 4 years ago

So points 1 & 2 can be met by introducing default language constants for SUCCESS="Success" and DANGER="Danger".

Why? They are not used in core. If it is for your own component then you should add them there

rogercreagh commented 4 years ago

Why?

Because the current core behaviour is inconsistent. alert-danger and alert-success are defined as alert classes in the core, and enqueueMessage presents a message of that type without bothering to provide a translation for the class. alert-error is an alias for alert-danger as a class but has a translation for msgtype 'error' there is no alert-message as an alias for alert-success but msgtype 'message' is translated to 'Message' for the heading and transposed to success for the class. there is no alias at all for alert-info so there is no simple way to produce a blue message with a different heading

This is all very illogical and smacks of something that has grown organically (long ago - since Mambo days?) and never been properly structured.

I accept that success and danger could be addressed by using component specific translations, but since the classes are part of the core and used across many components it would make sense to provide the translations in the core. It would also make sense to provide a simple way for a component to access the blue alert type without having to override the core language string for INFO as that could have undesired side effects. The green alert can be accessed by using msgtype success and providing a translation for SUCCESS, but why not provide it in the core as well. And the yellow message can be accessed by using anythingelse for the msgtype and providing a translation and optionally also a css override. This already provides full component specific extensibility. But success and danger and access to a different title for the blue message could most effectively be provided by the core whether or not the actual message types 'success' and 'danger' and (say) 'information' (for an alert-info class alias and a translation to 'Information') are currently used in the core.

Make the translations consistent with the classes.

ReLater commented 3 years ago

See also FOR JOOMLA 4: https://github.com/joomla-projects/custom-elements/issues/142#issuecomment-721110845

DelPoint commented 2 years ago

Well, I'm not sure what is used by core, and what doesn't. However, somehow I feel to concur @rogercreagh that something went sideways, not only with the colouring but also the HTML output.

Examining the code of message.php in \layouts\joomla\system\ found this: Array $alert does some re-mapping in line 20:

$alert     = [
    CMSApplication::MSG_EMERGENCY => 'danger',
    CMSApplication::MSG_ALERT     => 'danger',
    CMSApplication::MSG_CRITICAL  => 'danger',
    CMSApplication::MSG_ERROR     => 'danger',
    CMSApplication::MSG_WARNING   => 'warning',
    CMSApplication::MSG_NOTICE    => 'info',
    CMSApplication::MSG_INFO      => 'info',
    CMSApplication::MSG_DEBUG     => 'info',
    'message'                     => 'success'
];

IMHO the last item in the array is somehow reversed, should be 'success' => 'message'. Now, checking the code below directly that $alert, you'll see this:

// Load JavaScript message titles
Text::script('ERROR');
Text::script('MESSAGE');
Text::script('NOTICE');
Text::script('WARNING');`

We load language string for 'ERROR' here which is not used by this PHP file, neither by the messages.js in /media/system/js/, look around in Joomla.RenderMessages here: if (['success', 'info', 'danger', 'warning'].indexOf(type) < 0) { alertClass = (type === 'notice') ? 'info' : type; alertClass = (type === 'message') ? 'success' : alertClass; alertClass = (type === 'error') ? 'danger' : alertClass; alertClass = (type === 'warning') ? 'warning' : alertClass;

So, message.php loads language strings 'ERROR', 'MESSAGE', and 'NOTICE', while the JS will not use them on the output, and we don't load the lang strings for 'INFO' in the PHP file, which string is used by JS/PHP actually, and we can not load lang strings for 'danger' and 'success', because these are not defined in the language files at all. This results that out of 5 different type of messages (error, notice, warning, info, message) only one will be translated if J! front-end is not English.

That seems to me a bit messy.

Obviously, there are several ways to overcome this (overrides, or defining our own lang strings), but the current state is not I would like to agree to.

Ideas? Suggestions?