Skip to content

[3.x] Added some missing deprecations#30938

Merged
rdeutz merged 2 commits intojoomla:stagingfrom
alikon:patch-81
Feb 8, 2021
Merged

[3.x] Added some missing deprecations#30938
rdeutz merged 2 commits intojoomla:stagingfrom
alikon:patch-81

Conversation

@alikon
Copy link
Contributor

@alikon alikon commented Oct 6, 2020

Summary of Changes

Added some missing deprecations:

  • isAdmin()
  • isSite()
  • getCfg()

Testing Instructions

enable debug plugin
enable Log Depreated API
install some extensions that use one ofthose deprecated methods
or simply create a dummy sistem plugin like this one

class PlgSystemOldwaytest extends JPlugin
{
	function onAfterRoute()
	{
		$app = JFactory::getApplication();
		if ( $app->isAdmin() )
		{
			return;
		}
	}
}

Actual result BEFORE applying this Pull Request

no depreactions logged

Expected result AFTER applying this Pull Request

2020-10-06T09:41:08+00:00 WARNING 172.16.238.1 deprecated Joomla\CMS\Application\CMSApplication::isAdmin() is deprecated. Use JFactory->getApplication()->isClient('administrator') instead.

@HLeithner
Copy link
Member

I'm not sure if it's a good idea to do it without flood protection but maybe you could add "will be removed in Joomla! 4.0" ? Don't know how the order deprecation warnings are named

@zero-24
Copy link
Contributor

zero-24 commented Oct 6, 2020

What kind of flood protection do you have in mind? This is only added when you have debug and api deprecations enabled right?

@alikon
Copy link
Contributor Author

alikon commented Oct 6, 2020

This is only added when you have debug and api deprecations enabled right?

yes

@HLeithner
Copy link
Member

iirc we have some "log only once" if statements for other deprecations but maybe I'm wrong.

If this function is called really often it could lead to a performance penalty for 3.x series.

@zero-24
Copy link
Contributor

zero-24 commented Oct 6, 2020

If this function is called really often it could lead to a performance penalty for 3.x series.

Only once you have debug + api deprecation logging enabled right? Else nothing is written right?

@brianteeman
Copy link
Contributor

If this function is called really often it could lead to a performance penalty for 3.x series.

This only logs something if you enable debug and enable logging of deprecated api in the plugin. It is off by default. AND it warns you to only have it running for a short period of time. I guess you've never used it

@alikon
Copy link
Contributor Author

alikon commented Oct 6, 2020

If this function is called really often it could lead to a performance penalty for 3.x series.

yes that's true, but it is already like this when you enable LOG Api deprecations, and plus this should be done only on purpose

iirc we have some "log only once" if statements for other deprecations

i not aware about this.....

@sandewt
Copy link
Contributor

sandewt commented Oct 6, 2020

I have tested this item ✅ successfully on ecaa6c2

Joomla! 3.9.22-rc Release Candidate [ Amani ] 2-October-2020 11:00 GMT

Plugins: System - Debug > Log Deprecated API = Yes


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

@zero-24 zero-24 added this to the Joomla! 3.9.23 milestone Oct 6, 2020
@brianteeman
Copy link
Contributor

@alikon why are only your PR getting the hacktoberfest tag?

@alikon
Copy link
Contributor Author

alikon commented Oct 6, 2020

due to changed rules https://hacktoberfest.digitalocean.com/hacktoberfest-update
every pr should be marked with the label hacktoberfest-accepted to be eligible
feel free to send me or to any bug-squad members a list of pr's to be labelled as such

@brianteeman
Copy link
Contributor

PRs count if:
Submitted during the month of October AND (
  The PR is labelled as hacktoberfest-accepted by a maintainer OR
  Submitted in a repo with the hacktoberfest topic AND (
    The PR is merged OR
    The PR has been approved
  )
)

@alikon
Copy link
Contributor Author

alikon commented Oct 6, 2020

yes exactly these are the new rules

@brianteeman
Copy link
Contributor

In other words you do not need to add the label. but if you are going to choose that option then you should be applying it to all PRs not just yours or those that someone asks for the label. Most people dont know to ask.

I want everyone to plant a tree

@zero-24
Copy link
Contributor

zero-24 commented Oct 6, 2020

I want everyone to plant a tree

We have the hacktoberfest topic so that should not be a blocker.

@richard67
Copy link
Member

richard67 commented Oct 6, 2020

I don't join the hacktoberfest so about my PR(s) I don't care.

Update 2020-10-17: Meanwhile I've decided to join, too. Wanna plant a tree, too.

@alikon
Copy link
Contributor Author

alikon commented Oct 6, 2020

yes the label is just one of the options available, sometimes a pr need some time to be merged and /or approved
so the label in these case can be a shortcut

@brianteeman
Copy link
Contributor

so we dont need the label - thats good to know!!

@HLeithner HLeithner self-assigned this Oct 11, 2020
@ghost
Copy link

ghost commented Nov 25, 2020

I have tested this item ✅ successfully on ecaa6c2


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

@alikon
Copy link
Contributor Author

alikon commented Nov 25, 2020

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Nov 25, 2020
@Quy Quy modified the milestones: Joomla! 3.9.23, Joomla! 3.9.24 Nov 25, 2020
@rdeutz rdeutz merged commit e6cbda0 into joomla:staging Feb 8, 2021
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Feb 8, 2021
@alikon alikon deleted the patch-81 branch February 9, 2021 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants