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

[5.2] Deprecate WebApplication::$JComponentTitle #43304

Merged
merged 1 commit into from Apr 23, 2024

Conversation

HLeithner
Copy link
Member

@HLeithner HLeithner commented Apr 18, 2024

Summary of Changes

Deprecate the legacy parameter $JComponentTitle from WebApplication. This should have been done when the variable has been created. Sadly it has been missed.

Testing Instructions

No test needed

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on 247c409


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

@ChristineWk
Copy link

J 5.2.0-alpha1-dev
Code Review


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

@ChristineWk
Copy link

I have tested this item ✅ successfully on 247c409


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

@Quy
Copy link
Contributor

Quy commented Apr 18, 2024

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Apr 18, 2024
@wilsonge
Copy link
Contributor

The usage in mod_title goes back to at least joomla 1.6 and I suspect earlier (

$title = &JFactory::getApplication()->get('JComponentTitle');
). Whilst it definitely belonged in AdministratorApplication and not WebApplication, I think this needs better docs than just deprecated because core isn’t using it, we don’t know what plugins or admin modules might be using this from days gone old. Such as what is the correct method for plugins etc to set a page title. I believe when production made the rule of 2 major versions deprecation the deprecation also had to include a replacement method

@HLeithner
Copy link
Member Author

Replacement is set or get title Form the document AS documented in the deprecation.

@laoneo
Copy link
Member

laoneo commented Apr 19, 2024

Should you then not fallback to the document title here https://github.com/joomla/joomla-cms/pull/43304/files#diff-a35dff7f70b05500db42e3f582368ededb2d2477bbc955142c16a0038e184a6cR40, when this should be the replacement?

@HLeithner
Copy link
Member Author

Should you then not fallback to the document title here https://github.com/joomla/joomla-cms/pull/43304/files#diff-a35dff7f70b05500db42e3f582368ededb2d2477bbc955142c16a0038e184a6cR40, when this should be the replacement?

We do this when the variable gets removed in 7.0

$app->getDocument()->setTitle($title);

Until then we have to use the variable because we can't (or don't do) intercept when someone sets the JComponentTitle by hand in the application.

So yes in 7.0 we have to use document->getTitle() in the module.

@laoneo
Copy link
Member

laoneo commented Apr 19, 2024

I think this should be done now as fallback, something like:

$data['title'] = isset($this->getApplication()->JComponentTitle) ? $this->getApplication()->JComponentTitle : $this->getApplication()->getDocument()->getTitle();

@HLeithner
Copy link
Member Author

since this is only used for mod_title and only in combination with the toolbar::title function it's something which would be a behavior change which could be unexpected. ex. if your want to remove the title for you extension $app->JComponentTitle = ''; or null worked the last 15 years. If you change this now you can remove the variable and functionality directly and would have the same effect.

Extension developer have to set it in $doc->setTitle();.

only a thought I have I didn't proofed if this is a usecase

@laoneo
Copy link
Member

laoneo commented Apr 19, 2024

As it is an isset() check, '' or null would still work, as long as the variable exists.

@HLeithner
Copy link
Member Author

As it is an isset() check, '' or null would still work, as long as the variable exists.

I'm sorry but that's wrong

@laoneo
Copy link
Member

laoneo commented Apr 19, 2024

True, null is not covered

@pe7er pe7er merged commit 2ff6c04 into joomla:5.2-dev Apr 23, 2024
3 of 5 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Apr 23, 2024
@pe7er
Copy link
Contributor

pe7er commented Apr 23, 2024

Thanks @HLeithner !

@Quy Quy added this to the Joomla! 5.2.0 milestone Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants