Joomla! Issue Tracker - CMS

Download
Launch
Feeling Lucky

[#14250] - Use setDocumentTitle method of JViewLegacy to set document title

? Pending

User tests: Successful: Unsuccessful:

avatar joomdonation
joomdonation
25 Feb 2017

Pull Request for Issue # .

Summary of Changes

Since Joomla 3.6 , JViewLegacy has implemented the method setDocumentTitle https://github.com/joomla/joomla-cms/blob/staging/libraries/legacy/view/legacy.php#L854-L873 which is used to set document title base on site configuration. However, in many views in Joomla code base, we still use manual code, repeating code to set page title. This PR solves it

Testing Instructions

A real test would be check all views changes by this PR and make sure the document title is not changed before and after patch. However, honestly, I don't think we can expect that kind of test here because it would take much time

So I hope developers can help reviewing the code of the PR and confirm that it is expected changes. If you review code, please help reviewing every file carefully.

Documentation Changes Required

None

26e3232 25 Feb 2017 avatar joomdonation Typo
avatar joomdonation joomdonation - open - 25 Feb 2017
avatar joomdonation joomdonation - change - 25 Feb 2017
Status New ⇒ Pending
avatar joomla-cms-bot joomla-cms-bot - change - 25 Feb 2017
Category ⇒ Front End com_contact com_content com_finder com_newsfeeds com_tags com_users com_wrapper Libraries
avatar joomdonation joomdonation - change - 25 Feb 2017
Title
…
Patch 6
Use setDocumentTitle method of JViewLegacy to set document title
avatar joomdonation joomdonation - edited - 25 Feb 2017
avatar joomdonation joomdonation - change - 25 Feb 2017
Labels Added: ?
avatar zero-24
zero-24 - comment - 25 Feb 2017

looks good now from a cs persprctive ?

avatar joomdonation
joomdonation - comment - 25 Feb 2017

Again, many thanks for your help @zero-24. I realize that there are still changes needed to some other files (to remove un-used variables + minor optimization). In case there are CS changes needed, I would love to receive your help again :)

avatar zero-24
zero-24 - comment - 25 Feb 2017

no problem just ping me ?

avatar joomdonation
joomdonation - comment - 26 Feb 2017

@wilsonge @rdeutz This PR makes changes to many files, should I spit it into smaller PRs (each PR for one component) for easier code review and we can get human testing, too?

avatar rdeutz
rdeutz - comment - 26 Feb 2017

if you had 200 than yes but 20 changed files is ok

avatar joomdonation
joomdonation - comment - 26 Feb 2017

OK. Thanks. I will leave it as how it is, then

avatar n9iels n9iels - test_item - 2 Mar 2017 - Tested successfully
avatar n9iels
n9iels - comment - 2 Mar 2017

I have tested this item ✅ successfully on 823693e

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

avatar joomdonation joomdonation - change - 14 May 2017
Status Pending ⇒ Closed
Closed_Date 0000-00-00 00:00:00 ⇒ 2017-05-14 01:39:23
Closed_By ⇒ joomdonation
avatar joomdonation joomdonation - close - 14 May 2017
avatar n9iels
n9iels - comment - 14 May 2017

@joomdonation What is the reason to close this PR?

avatar joomdonation
joomdonation - comment - 14 May 2017

@n9iels Honestly, a PR which changes multiple files like this would be hard to get testers and proper testings

Also, I was thinking about introduce a new method which set both page title and page meta data (which basically including this block of code as well https://github.com/joomdonation/joomla-cms/blob/0031973e956bc83700140cb905e4a963c195cd45/components/com_content/views/form/view.html.php#L163-L176 )

So I think it is better to close this PR and do proper coding in Joomla 4. Thanks for reviewing the changes though :)

Add a Comment

Login with GitHub to post a comment