JM this is a bad solution because it stops you using the Javascript method to render a message. That requires the "system-message-container" div https://github.com/joomla/joomla-cms/blob/staging/media/system/js/core-uncompressed.js#L126

I dont see the need for this PR as mentioned on the tracker

The proposed solution does NOT remove the system-message-controller div, only it's contents.
It tidies up the code by making no system message the same as an empty array of system messages, a defensive coding technique that should have been used in the first place.
Brian's comment seem to amount to 'if it's not a problem in the bundled templates then it doesn't matter'. This is at odds with the Joomla standard for coding that encourages the elimination of uncontrolled white space by removing closing php tags at the end of files. This is just another example of uncontrolled white space.

Thats not what i am saying. I am saying please show an example of this problem in the real world as to date I have never seen it

@doorknob2 Incorrect. If the message list is empty then system-message-controller div is removed. But if the message list is empty but a message is added by JS then it will fail after this patch

@doorknob2 The problem is actually that this PR doesn't do what the tracker originally suggested. This PR moves the if up one step, which removes everything. But as George said you need to keep the <div id="system-message-container"></div> for sure.
The tracker originally suggest to only remove the inner <div id="system-message"></div> in case no message is set, but still showing the outer container.
Looking at the code, this is what probably was intended with the if (is_array($msgList)). But since this check returns true for an empty array, the div is always shown. And thus you can't for example apply margins to that div like you should be able to do.

So there is an issue and the proposed fix in the tracker would work, but this PR is wrong.

@infograf768 Can you update your PR with the correct fix?

IMO, and if i understand the issue correctly, the problem is not with layouts/joomla/system/message.php but rather the template. For example, protostar has <jdoc:include type="message" /> while not checking first checking $app->getMessageQueue(); See https://github.com/construct-framework/bootstruct/blob/master/index.php#L318 for an example of how I have dealt with this. Maybe that can be used in layouts/joomla/system/message.php¿

@betweenbrain What you did in bootstruct is actually wrong as well.
As George pointed out, we have a JavaScript function which can render system messages on client side (https://github.com/joomla/joomla-cms/blob/staging/media/system/js/core-uncompressed.js#L126). For that to work a container with the id system-message-container needs to be always present on the page, even if no message is shown. If it's missing, the JavaScript function will fail.

Interesting, as that is inconsistent behavior with other <jdoc include/> statements. It also relies on markup that won't necessarily be rendered as the message layout can be overrides. I'd suggest that the JavaScript function might need to be evaluated to be as well to be more robust and have fewer dependencies on this markup allowing for more flexible implementation.

@Bakual @wilsonge could you help me understand why we have a JavaScript function to render the message? I never knew we had that and was a bit confused by the earlier reference.

AJAX requests that have messages could use that method.

@betweenbrain @mbabker
You don't even need AJAX. Our JavaScript validation uses it for example.
To test, go edit an article and remove the title. Save it. You will see that a message is written into the container. Remove the container and it will no longer work.

Updated PR to move the conditional after the <div id="system-message-container">

@mbabker @Bakual thanks for the clarification about the JavaScript function.

At this point in time, @infograf768's solution does seem to be the best way to approach this.

That being the case, I am concerned about Joomla.renderMessages targeting an empty DOM element that may or may not exists. With layouts/joomla/system/message.php we've given users the ability to fully customize the markup that Joomla.renderMessages depends on. Historically, I am aware of other template devs that have implemented similar solutions like I did in the Construct series and check the message queue for a message so that an empty div isn't rendered. This is all beyond the scope of this PR, but it has shown some of the issues with this dependency.

@betweenbrain Also with JHtml::script(); we've given users the opportunity to override said class in the javascript if they want in their template ;) If people want to play with the message container that's fine - but as soon as you override core output there can always be consequences. Anyhow as you say out of context of this pr

The only thing left now is this is also missing in Beez and the code is different from Isis. (meant the condition)

To my knowledge it's not really an issue in Beez since Beez doesn't apply special formatting to that div.

Where do we stand now on this specific PR. Ready to merge?

Where do we stand now on this specific PR. Ready to merge?

Fine for me.

Very bad for code quality: three different implementations for no reason!

Very bad for code quality: three different implementations for no reason!

Can you be more specific? Would you be able to propose an improvement? I'm certainly very interested in improving the code quality.

Yes where are the 3 different implementations??

Yes where are the 3 different implementations??

I guess:

• default
• Isis
• Beez3

As already mentioned:
1. beez3 : if (is_array($msgList))
2. html (this PR) : <?php if (is_array($msgList) && !empty($msgList)) : ?>
3. isis : if (is_array($msgList) && (count($msgList) >= 1))

At least the beez3 and html should be the same, assuming logic needs to be the same, markup might be different. Or in other words: when it was wrong for protostar, it is wrong for beez3 as well considering it is not a template difference.

So beez3 should be updated to use this logic or the ISIS logic. Actually i have to say I might actually prefer the ISIS logic - it makes a bit more sense in the context of a number of messages.... But you're right we should make all 3 consistent

Considering the issue was solved in isis, don't see any reason to implement something different in html or beez3. That is what I call code quality and consistency.

When the implementation of isis is not according to standards or there are reasons to implement this differently, it still should be done the same way.

Why not change them to: count($msgList) > 0

Why not change them to: count($msgList) > 0

You could actually just use
if (is_array($msgList) && $msgList) : or if you want to be a bit more verbose if (is_array($msgList) && count($msgList)) :

It is to note that this is only a question of coding preferences/consistency. Our codestyle doesn't say anything about what to use.
In the case of Beez3, the check isn't needed (although it wouldn't hurt of course). If someone wants to do a PR, I'm not speaking against

Well as part of the PR I guess you move across to using a JLayout

If it is a bug for protostar it is a bug for beez3 because that was based on that code!

Maybe it is time to set the codestyle for it. For sure introducing different ways to do the same thing is bad coding standard! Have seen several requests for changing code to be consistent, consider that a good thing. Suprised about the objections!

I don't see how JLayout is going to help here, but I don't care about Beez3. And if it is so difficult to convince people to do the right thing, I'll stop bothering.

If it is a bug for protostar it is a bug for beez3 because that was based on that code!

Protostar doesn't have an override. It uses the default output. In Protostar itself it wasn't an issue. But since that default output is used by many templates, it could create issues for them. They could of course override it as well, but it makes sense fixing it in the default output.

Maybe it is time to set the codestyle for it. For sure introducing different ways to do the same thing is bad coding standard! Have seen several requests for changing code to be consistent, consider that a good thing. Suprised about the objections!

I'm all for standardising the code. It's sometimes just not that easy to find a consent how to do something. For example I prefer if ($variable) but I know others prefer if (!empty($variable)). Technically it does (almost) the same.
Feel free to start a PR here: https://github.com/joomla/coding-standards

I don't see how JLayout is going to help here, but I don't care about Beez3. And if it is so difficult to convince people to do the right thing, I'll stop bothering.

JLayout would allow to write the HTML directly instead of wrapping it all into a variable and using \nand \t all over the place. Compare https://github.com/joomla/joomla-cms/blob/staging/layouts/joomla/system/message.php with https://github.com/joomla/joomla-cms/blob/staging/templates/beez3/html/message.php and you see what I mean.

Would the JLayout be able to produce both protostar and beez3 html? Properly formatted for source code reading of page?

You would do an override of the layout if it needs to be different. Then it should work.

See PR #3406