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

Fixed issue with countModules also counting empty modules #7594

Closed
wants to merge 1 commit into from
Closed

Fixed issue with countModules also counting empty modules #7594

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Jul 31, 2015

Templates use the $this->countModules('myposition') to determine whether or not to show a position (and surrounding html).
However, when a module is empty (no content), the countModules is not affected.
This can result in positions being shown with no modules in it.

This PR fixes the issue by only counting the modules that do not have an empty content.

Testing Instructions

Easiest way to test with an empty module is to use Advanced Module Manager, as that will make the content truly empty even if the module has an empty <p></p> tag.
So just make a Custom HTML Module and leave the content area empty. Assign it to a module position (that has no other modules assigned to it). Like manually entering testempty in the module position field. And switch on the 'Hide if Empty' option.

Or create your own module that returns an empty content.

Then in your template place this somewhere:

<?php
echo "\n\n<pre>==========================\n";
echo 'Module count for position "testempty": ' . $this->countModules('testempty');
echo "\n==========================</pre>\n\n";
?>

Before this patch it will show a count of 1, after a count of 0.

Templates use the ```$this->countModules('myposition')``` to determine whether or not to show a position (and surrounding html).
However, when a module is empty (no content), the ```countModules``` is not affected.
This can result in positions being shown with no modules in it.

This PR fixes the issue by only counting the modules that do not have an empty content.

#### Testing Instructions
Easiest way to test with an empty module is to use Advanced Module Manager, as that will make the content truly empty even if the module has an empty ```<p></p>``` tag.
So just make a Custom HTML Module and leave the content area empty. Assign it to a module position (that has no other modules assigned to it). Like manually entering ```testempty``` in the module position field.

Then in your template place this somewhere:
```
<?php
echo "\n\n<pre>==========================\n";
echo 'Module count for position "testempty": ' . $this->countModules('testempty');
echo "\n==========================</pre>\n\n";
?>
```

Before this patch it will show a count of 1, after a count of 0.
@brianteeman
Copy link
Contributor

What if someone is using the module JUST to display the title

On 31 July 2015 at 11:05, Peter van Westen notifications@github.com wrote:

Templates use the $this->countModules('myposition') to determine whether
or not to show a position (and surrounding html).
However, when a module is empty (no content), the countModules is not
affected.
This can result in positions being shown with no modules in it.

This PR fixes the issue by only counting the modules that do not have an
empty content.
Testing Instructions

Easiest way to test with an empty module is to use Advanced Module
Manager, as that will make the content truly empty even if the module has
an empty

tag.
So just make a Custom HTML Module and leave the content area empty. Assign
it to a module position (that has no other modules assigned to it). Like
manually entering testempty in the module position field.

Then in your template place this somewhere:

==========================\n"; echo 'Module count for position "testempty": ' . $this->countModules('testempty'); echo "\n==========================\n\n"; ?>

Before this patch it will show a count of 1, after a count of 0.

You can view, comment on, or merge this pull request online at:

#7594
Commit Summary

  • Fixed issue with countModules also counting empty modules

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#7594.

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

@ghost
Copy link
Author

ghost commented Jul 31, 2015

As you can see in the module chromes of protostar and pretty much all templates:

function modChrome_well($module, &$params, &$attribs)
{
    if ($module->content)
    {
        ...
    }
}

So templates are only outputting the module if it has content anyway.

@brianteeman
Copy link
Contributor

Sorry thats not true - a title is part of content

home

@brianteeman
Copy link
Contributor

Thats true at least for a custom html module - not tested others

@ghost
Copy link
Author

ghost commented Jul 31, 2015

Then this patch won't affect that and will still show it.

@Fedik
Copy link
Member

Fedik commented Jul 31, 2015

sorry I not agree with this changes also,
if the module exist and published and available on current page then it must be counted, no matter it empty or not

@ghost
Copy link
Author

ghost commented Jul 31, 2015

Ok, I'll just hack my way around the weird issues in Joomla again.
Closed.

@ghost ghost closed this Jul 31, 2015
@Duke3D
Copy link
Contributor

Duke3D commented Jul 31, 2015

Can we re-open this please?


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

@mbabker
Copy link
Contributor

mbabker commented Jul 31, 2015

At best this is a "discuss for 4.0" item; re-opening it and trying to push it for 3.x is a no go IMO as it causes a pretty big behavioral change (not rendering modules if the content is empty which can affect site layouts pretty bad). And honestly, I don't think there's a bug in the current behavior; if the conditions are to count all published modules without considering their content then this method behaves as intended.

@ghost
Copy link
Author

ghost commented Aug 1, 2015

Please see: #7606

@ghost ghost deleted the patch-2 branch August 1, 2015 08:04
@pinta83
Copy link
Contributor

pinta83 commented Aug 24, 2015

I have to agree with nonumber with this.... I'm with Joomla since v1, have build dozens of sites, and this is a very usefull feature... ATM I have to do workarounds in my template....


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

@brianteeman
Copy link
Contributor

OK I was completely wrong on this one and now fully understand the issue. This is the scenario I faced and perhaps it will help others to understand it.

Let us say that if $this->countModules('myposition') is true we display a right hand column but if the count is false we dont.

Now if you have a split menu or any dynamic module such as Article-Categories that is set to be published on every page the count will always be true BUT if there are no menu items to be displayed on the page then you end up with a completely empty right hand column.

(sadly the proposed fix didnt work for me)

@pinta83
Copy link
Contributor

pinta83 commented Nov 5, 2015

Yeah, the point was to determine if the modules not only exist in that position, but also produce any content.... So $this->countModules('myposition') only checks for modules, not for content.
And then imagine a screnario where you etc build a sidebar because module exists, but there really isn't any content in the module it self, resulting in an empty sidebar....
It would be a nice feature although i don't really see how to accomplish this without rendering modules twice...

@ghost
Copy link
Author

ghost commented Nov 5, 2015

As discussed in #7606 it isn't possible for Joomla to check this the way I want to.
For the countModules to know if a module is empty, it would need to render the modules first.
That is not how the order of things in the template rendering work.
Also, you can't just check for the $module->content, as only html modules have that.

Also you run into other side issues:
What is empty? Is an empty <p></p> empty? What about an empty list or div? And if so, also when they have classes? These elements might get replaced/filled via ajax.
What if a module has no actual html output but only adds scripts/styles to the document?

So as far as I can see it, this issue will be there as long as the way Joomla does the template/module rendering stays as it is.
If you want to change that you open up a whole can of B/C issues.

@brianteeman
Copy link
Contributor

I was really just posting back to acknowledge I was wrong

@ghost
Copy link
Author

ghost commented Nov 5, 2015

OK :)

I was wrong too in thinking I could fix it!

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants