[#34320] - [4] hits - Move rendering from outside of the model
- Fixed in Code Base
- 6 Jun 2021
- Medium
- Build: 4.0-dev
- # 34320
- Diff
- PhilETaylor:movelogic
User tests: Successful: Unsuccessful:
Summary of Changes
#34257 (@brianteeman) added rendering in the helper (which is used as a proxy to the model, for a getList method calling a getItems in the model.
This is the wrong place to be rendering output.
This PR moves the output rendering to the parent module file.
Testing Instructions
Add the Articles - Most Read module to your home page and view it with Record Hits enabled and disabled in Article Options.
and/or code review
Actual result BEFORE applying this Pull Request
Same as before this PR
Expected result AFTER applying this Pull Request
Same as before this PR but correct code placement.
Documentation Changes Required
None.
This PR will conflict with #34318 eventually - when it does I'll fix it before merge
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Modules Front End |
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34320.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34320.
| Status | Pending | ⇒ | Ready to Commit |
| Labels |
Added:
?
|
||
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34320.
Not that bothers to much but shouldn't this be in it's own layout then?
yes probably, but its overkill as its
- Used once, in one place
- more function calls, file loads, just to echo out a single string
:)
@PhilETaylor Could you fix the conflict in file modules/mod_articles_popular/src/Helper/ArticlesPopularHelper.php? It comes from your other, merged PR #34318 .
| Labels |
Added:
Conflicting Files
?
|
||
Done.
@PhilETaylor Thanks. In fact this PR had the correction of the other one already, as I could see, so the last commit was just a clean branch update.
Previous test results are still valid. I've restored them in the issue tracker.
| Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2021-06-06 21:10:08 |
| Closed_By | ⇒ | chmst | |
| Labels |
Added:
?
Removed: Conflicting Files ? |
||
Thanks!
This PR will conflict with #34318 eventually - when it does I'll fix it before merge