Cleanups, fixes and a bit of optimizations for site/components batch #1#12290
Cleanups, fixes and a bit of optimizations for site/components batch #1#12290wilsonge merged 5 commits intojoomla:stagingfrom frankmayer:site-com_ajax-com_banners-com_config
Conversation
- com_ajax - com_banners - com_config Note: This is a single commit bundling all types of changes, since PR #12261 which had detailed commits, was rejected as a whole
| } | ||
| } | ||
| elseif ((is_array($categoryId)) && (count($categoryId) > 0)) | ||
| elseif (is_array($categoryId) && (count($categoryId) > 0)) |
There was a problem hiding this comment.
why not remove the parenthisis on count too?
| OR a.own_prefix=0 | ||
| AND cl.own_prefix=0 | ||
| AND $prefixCondition | ||
| SQL; |
There was a problem hiding this comment.
as said, i prefer using the API here, but that is just my opinion.
There was a problem hiding this comment.
this one i think it should be a mantainer decision.
@wilsonge please check, we normally use the api and the quoteName as PLT meeting notes.
for the rest i think it's fine now
There was a problem hiding this comment.
I don't know the scope of the PLT meeting notes, but while the use of the query-builder might be OK in most cases, it might be suboptimal when queries are running in loops, like for example in the finder (indexer, lexer, etc.). My proposed way might be useful at least in such cases as the HEREDOC is also much faster than string or array concatenation via implode().
There was a problem hiding this comment.
I'd definitely prefer that we do. But this is code specifically for MsSQL right? I remember this being a total nightmare in the past to get sorted. We have to be much more careful here than in standard queries with the query builder.
| // Get the template form. | ||
| if (!$form->loadFile($formFile, false, '//config')) | ||
| { | ||
| throw new Exception(JText::_('JERROR_LOADFILE_FAILED')); |
There was a problem hiding this comment.
if you remove one if block you have to remove one ident tab inside the if block
There was a problem hiding this comment.
I know and I am doing that. That many changes, sometimes things slip through. 😄
|
This looks fine on review other than the query which I straight skipped because I need to go do some reading up on |
|
Should I revert the query change, so that this PR can go through? It's a pity that the other changes are held back because of the query. |
|
you should not mix Code Style, bug fixes and other changes in one PR. A PR should have one topic and nothing more that makes it easier to test and it gets merged faster |
|
Yes, the problem with that is, that for those kinds of changes, there would probably be need for hundreds of PRs (and therefore branches) if those are not somehow grouped to digestible chunks, which I tried to do with this set of PRs. |
|
I have tested this item ✅ successfully on eec22d1 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12290. |
# Conflicts: # components/com_banners/models/banners.php
|
Fixed some conflicts that had occurred in the meantime. |
Note: This is a single commit bundling all types of changes, since PR #12261 which had detailed commits, was rejected as a whole