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

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

Merged
merged 1 commit into from Feb 1, 2015

Conversation

Hackwar
Copy link
Member

@Hackwar 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
Copy link
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
Copy link
Member Author

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
Copy link
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
Copy link

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
Copy link
Contributor

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 This Pull Request is Ready To Commit 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
Fix for: Banners matched only by tags results in SQL error
@roland-d roland-d merged commit 7533bc6 into joomla:staging Feb 1, 2015
@zero-24 zero-24 removed the RTC This Pull Request is Ready To Commit label Oct 14, 2015
@Hackwar Hackwar deleted the com_banners_category branch April 27, 2019 07:46
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.

Banners matched only by tags results in SQL error
7 participants