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

Query-fix in com_content article modal #2795

Merged
merged 8 commits into from Feb 15, 2014
Merged

Conversation

Hackwar
Copy link
Member

@Hackwar Hackwar commented Jan 15, 2014

This change prevents the query from being executed when the value is 0. There is never an article with ID 0 and especially when there are more than one modal select per page, you have several unnecessary queries.

This change prevents the query from being executed when the value is 0. There is never an article with ID 0 and especially when there are more than one modal select per page, you have several unnecessary queries.
@Bakual
Copy link
Contributor

Bakual commented Jan 15, 2014

Can you add the tracker and some testing instructions?

@Hackwar
Copy link
Member Author

Hackwar commented Feb 7, 2014

@Bakual
Copy link
Contributor

Bakual commented Feb 7, 2014

especially when there are more than one modal select per page, you have several unnecessary queries.

Do we have this in core somewhere? Just asking for testing. The only place I know we have one modal is the menu item form for the single article view.

catch (RuntimeException $e)
$title = false;

if ((int) $this->value > 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a check if ((int) $this->value > 0) here? Imho if ((int) $this->value) would be sufficient.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the value is negative, its equally wrong, but will pass through the if()... Its not strictly necessary, but doesn't hurt either....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there even a case where it could possibly be a negative? I don't think so.
It's true that it doesn't hurt. It's just not needed.

@Hackwar
Copy link
Member Author

Hackwar commented Feb 7, 2014

This is in multilang sites, where you have this field for every language in your site. So a site with 5 languages will have 4 of these fields in every new article.

@Bakual
Copy link
Contributor

Bakual commented Feb 7, 2014

This is in multilang sites, where you have this field for every language in your site. So a site with 5 languages will have 4 of these fields in every new article.

Ah thanks!

@infograf768
Copy link
Member

Same should be done for contacts, newsfeeds, categories I guess.

@Hackwar
Copy link
Member Author

Hackwar commented Feb 11, 2014

Better? :-)

@infograf768
Copy link
Member

There is a conflict with the category one

Conflicts:
	administrator/components/com_categories/models/fields/modal/category.php
@Hackwar
Copy link
Member Author

Hackwar commented Feb 11, 2014

Should be good now.

JError::raiseWarning(500, $e->getMessage());
$db = JFactory::getDbo();
$db->setQuery(
'SELECT title' .
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be a douchebag can we update these queries to use JDatabaseQuery whilst we're at it? Or would you rather that in a separate PR?

@infograf768
Copy link
Member

@wilsonge
Let's indeed normalise all these queries (not only these) in another PR.

infograf768 added a commit that referenced this pull request Feb 15, 2014
Query-fix in com_content article modal
@infograf768 infograf768 merged commit 4219e4d into joomla:staging Feb 15, 2014
Bakual pushed a commit to Bakual/joomla-cms that referenced this pull request May 12, 2014
Query-fix in com_content article modal
@Hackwar Hackwar deleted the patch-1 branch January 6, 2016 11:30
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

5 participants