[#10851] - Code review on com_content component
- Closed
- 4 Sep 2016
- Medium
- Build: staging
- # 10851
- Diff
- mbabker:review-content-component
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
User tests: Successful: Unsuccessful:
This PR is a general overview and cleanup of some code structure and doc blocks in the admin com_content component. Changes of note:
- Doc blocks cleaned up some to follow standard
- Return the results of the view's
display()methods to be consistent with the documented return - Not using variables for items only referenced once
- Use
Joomla\Utilities\ArrayHelperinstead ofJArrayHelper - Removed empty override methods (the parent class' method is also empty)
Testing Instructions
The admin com_content component should continue to function correctly
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Labels |
Added:
?
|
||
| Category | ⇒ | Code style Components |
Works fine after applying patch.
Tested using Manual and Automatic testing using Gherkin Scenarios
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10851.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10851.
| Status | Pending | ⇒ | Ready to Commit |
| Labels |
Added:
?
|
||
I was going through the commit and came across this code at line pointed in the link above
$app = JFactory::getApplication();
$assoc = JLanguageAssociations::isEnabled()
// Check if article is associated
if ($this->getState('article.id') && $app->isSite() && $assoc)
Can this be replaced by this?
// Check if article is associated
if ($this->getState('article.id') && JFactory::getApplication()->isSite() && JLanguageAssociations::isEnabled() )
And one more question
Is the files under administrator/com_content/views/article/tmpl, administrator/com_content/views/articles/tmpl, administrator/com_content/views/featured/tmpl also overviewed in this PR?
Layout files are purposefully untouched. Also untouched are changes made after I originally created this commit many months ago. Right now it's fine as is.
Did a code review and all looks good except that one issue. Haven't tested yet due to merge conflicts. Once resolved I will test further.
| Milestone |
Added: |
||
| Status | Ready to Commit | ⇒ | Information Required |
| Labels | |||
| Milestone |
Removed: |
||
| Labels |
Removed:
?
|
||
| Status | Information Required | ⇒ | Pending |
@roland-d can you test this now please
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10851.
Seems like we hit another merge conflict :-/
| Category | Code style Components | ⇒ | Administration Components Code style |
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-09-04 01:02:16 |
| Closed_By | ⇒ | wilsonge |
I have tested this item✅ successfully on 76a78d7
No issues. Everything works as it is.
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10851.