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

Fix for: Banners matched only by tags results in SQL error #5107

Merged
merged 1 commit into from Feb 1, 2015

Conversation

Projects
None yet
7 participants
@Hackwar
Member

Hackwar commented Nov 14, 2014

The category table is falsely included/not included. Long story short, this fixes #5000 and is a better fix than #5098 See there for testing instructions.

@waader

This comment has been minimized.

Contributor

waader commented Nov 19, 2014

I can not bring together what started in this forum thread - http://forum.joomla.org/viewtopic.php?f=621&t=731120&view=previous#p2841883 (mentioned in #5000) - with what you did in your patch. In other words: I cannot replicate the original problem.

So the display of "module banner" depends basically on three criteria (I ignore the menu assignment, as it was set to "on all pages" in all of my tests):

  • client (I tested with the default setting - "no client")
  • category and
  • Search by Meta Keyword

When applying your patch the "categories" table is not joined when the default setting - "all categories" - is set. Without your patch the "categories" table is always joined. I my opinion the wording "all categories" implies that the table "categories" is joined. Otherwise is should be named "not used" or something like that.

The forum thread described a problem with the "Search by Meta Keyword". So I tested this with the following procedure using the current stage branch:

  • create an article and assign a keyword in "Meta Keywords"; Link the article to a menu item.
  • create a banner and assign the same keyword to "Meta Keywords".
  • create a module banner and set "Search by Meta Keyword" to yes. The fields client and category are not touched.
  • go to the frontend a select the menuitem with the linked article.

Result: the banner is not displayed but it should.

What I found out was, that a client must be selected in the banner, than the banner get´s displayed. This is an error as the client is not a mandatory field in the banner. It should function without assigning a client.

Apart from that the "Search by Meta Keyword" works for me.

Maybe I got it totally wrong. In this case, please advise.

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

@Hackwar

This comment has been minimized.

Member

Hackwar commented Dec 25, 2014

The filter for categories does not have anything to do with the categories table. Basically, you are only saying that you either want to have banners from all categories or just specific categories. In the first case, we don't have to do anything, in the second case we only have to compare the category ID of the banner with some ID that we saved before. So we don't need any data from the categories table.

However, if we filter by category AND a tag/metakey/whatever, we are trying to filter also by category metadata and that fails before this patch. Please test again.

@waader

This comment has been minimized.

Contributor

waader commented Jan 15, 2015

So here is my next try. The ingredients:

  • add a banner client
  • add a banner category and optionally enter a "tag" in the "Meta Keys" field eg. test1
  • add a banner and assign the client and the category and optionally enter a "tag" in the "Meta Keys" field eg. test1

You have to enter a "tag" in the banner or/and the banner category.

  • add a banner module with the following parameters:
    • Client=No client
    • Search by Tag=Yes
    • Randomize=Sticky, Randomize
    • position=Banner in Protostar
  • add an article and enter a "tag" in the "Meta Keys" field eg. test1
  • assign the article to a menu item
  • go to the frontend and invoke the menu item

Before the patch you should see notices like:

Warning: Invalid argument supplied for foreach() in joomla34\components\com_banners\models\banners.php on line 201

After the patch the banner is display correctly.

Note that you have to assign a client to the banner otherwise the banner will not show up in the frontend. As the client is not a mandatory field in the banner it should also work without assigning a client.

Anyway: the problem is solved
@test success!

#cjtop can you please check this patch also?

@wmgraf

This comment has been minimized.

wmgraf commented Jan 31, 2015

@test success!
Before the patch I saw the described notices.
After applying the patch the banner module shows not any longer that notices.
And on the fitting article including the keyword the banner is displayed correctly.

@brianteeman

This comment has been minimized.

Contributor

brianteeman commented Jan 31, 2015

Two good tests setting RTC


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

@brianteeman brianteeman added the RTC label Jan 31, 2015

@roland-d roland-d added this to the Joomla! 3.4.0 milestone Feb 1, 2015

roland-d added a commit that referenced this pull request Feb 1, 2015

Merge pull request #5107 from Hackwar/com_banners_category
Fix for: Banners matched only by tags results in SQL error

@roland-d roland-d merged commit 7533bc6 into joomla:staging Feb 1, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@zero-24 zero-24 removed the RTC label Oct 14, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment