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

Including core_params field in SELECT #6792

Closed
wants to merge 2 commits into from
Closed

Including core_params field in SELECT #6792

wants to merge 2 commits into from

Conversation

fly1ted
Copy link

@fly1ted fly1ted commented Apr 17, 2015

In my project I needed to access attribs field from #__content which is stored as core_params in #__ucm_content. This commit will allow to access this field when outputting tags related articles. It's especially useful if you have custom parameters associated with your article.

In my project I needed to access `attribs` field from #__content which is stored as `core_params` in #__ucm_content. This commit will allow to access this field when outputting tags related articles. It's especially useful if you have custom parameters associated with your article.
@brianteeman
Copy link
Contributor

Can you supply some ample code to make it easier for people to test this
please

On 17 April 2015 at 09:11, Oleksandr Panasenko notifications@github.com
wrote:

In my project I needed to access attribs field from #__content which is
stored as core_params in #__ucm_content. This commit will allow to access
this field when outputting tags related articles. It's especially useful if

you have custom parameters associated with your article.

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

#6792
Commit Summary

  • Including core_params field in SELECT

File Changes

Patch Links:


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

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

@fly1ted
Copy link
Author

fly1ted commented Apr 17, 2015

So, basically there's only one line added, it has effect on gettagitemsquery() method defined at /libraries/cms/helper/tags.php -> line 467 and referenced only in one place: Tag model /components/com_tags/models/tag.php -> line 147. With this change the contents of the object returned in response to call getItems() of this model will be added with one more field core_params. Thus in /components/com_tags/views/tag/tmpl/default.php elements under $this->items would have this field which isn't available now.

@Bakual
Copy link
Contributor

Bakual commented Apr 17, 2015

Ouch, that query looks awful already...

Looking at the other selects I don't think that will work. The whole query is a group one, the #__ucm_content is a joined table and may or may not return multiple matching rows. The other selects all use MAX() to make sure only one row is returned.

@fly1ted
Copy link
Author

fly1ted commented Apr 17, 2015

Yes, you're right on that query, but $query->select() will just add one more parameter to SELECT, so I don't see what bad could happen in this case. It works for me: one more field as expected.

default

@Bakual
Copy link
Contributor

Bakual commented Apr 17, 2015

I don't know if it's needed or not. But all other cases where something is fetched from the same table (c.) in that query has it wrapped in MAX().
Most likely one of the joins in that query may return more than one row in some cases, and then you get duplicate results if you haven't aggregated the fields.

@fly1ted
Copy link
Author

fly1ted commented Apr 17, 2015

Just checked, it works on multiple elements too. I have 3 articles matching a tag: all have core_params field.

@fly1ted
Copy link
Author

fly1ted commented Apr 17, 2015

I think it's needed because we should have access to all the parameters of an article whether it's returned. In other places: content modules or com_content views we have access to attribs field. I think in case of tags this shouldn't be different.

@fly1ted
Copy link
Author

fly1ted commented Apr 17, 2015

I've tried to brace core_params with MAX, it still works. Do you think it'll work in this case?

@Bakual
Copy link
Contributor

Bakual commented Apr 17, 2015

I'd say either all of the selects from c. need to be wrapped in MAX() or none of them. But having one not wrapped and the other are wrapped sure is wrong.
I honestly don't know if it's needed or not, but it should be consistent for sure.

@fly1ted
Copy link
Author

fly1ted commented Apr 17, 2015

Ok, I'll add another commit with the MAX, and you'll see merge it or not. Btw, is it possible to edit this pull request to reflect new changes with our discussion or I need to create a new one?

Now it includes MAX to be consistent with other fields.
@Bakual
Copy link
Contributor

Bakual commented Apr 17, 2015

The PR updates automatically with your new commit 😄

If you edit your first post and add detailed testing instructions, then it will be tested faster and could be merged.

@ghost
Copy link

ghost commented May 24, 2015

@test SUCCESS
Tested with staging package of today.

Some test instructions because @oldrpan didn't provide them:

  • Create menu item of type Tagged Items and select tag(s). Save menu item.
  • Open several Articles, Categories, Newsfeeds, Contacts, Contact Categories, whatever and add this tag to these items.
  • Set some individual options/parameters if you want to. Add image in categories and so on.
  • Save item(s).
  • Now a small core hack in
/components/com_tags/views/tag/tmpl/default_items.php

Directly after line 63

<?php foreach ($items as $i => $item) : ?>

add a new line

<?php echo 'Core_Params:<br /><pre>' . $item->core_params . '</pre><br />'; ?>
  • Open your new menu item in frontend (Protostar).
  • Properties core_params are all empty.

core_params_empty

  • Apply patch, reload page:

core_params_not_empty

  • These params are also important e.g. to be able to display category images that are saved in field core_params.

@coolcat-creations
Copy link
Contributor

Tested like described and successfull :)


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

@zero-24 zero-24 added the RTC This Pull Request is Ready To Commit label Jun 17, 2015
@zero-24 zero-24 modified the milestone: Joomla! 3.4.4 Jul 2, 2015
@Kubik-Rubik
Copy link
Member

Thank you @oldrpan! Merged.

@zero-24 zero-24 removed the RTC This Pull Request is Ready To Commit label Oct 14, 2015
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

6 participants