Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Deprecate application loading in JFactory::getApplication() #1729

Merged
merged 1 commit into from

6 participants

@mbabker
Owner

JFactory::getApplication() is heavily tied in to working with the legacy JApplication class, making it nearly impossible to properly refactor the method to work with the newer JApplicationBase classes. Since JFactory::getApplication() is heavily used to retrieve the application object, instead of deprecating and removing the method completely, instead I propose to deprecate its code to handle instantiating an application instance, in this case, JApplication. In 13.3 when JApplication is removed from the legacy tree, the method will be converted to throwing an Exception if the application object in JFactory is not set by applications. This allows this heavily used API to continue to be used to globally retrieve the application object.

As well, I've added a check to ensure that JApplication is present if getApplication() is actually loading an instance. Since it is in the legacy tree, there is the chance it is not present. An Exception is thrown in this instance.

@elinw

This makes a lot of sense to me.

libraries/joomla/factory.php
@@ -96,12 +96,29 @@ public static function getApplication($id = null, array $config = array(), $pref
{
if (!self::$application)
{
+ /*
+ * @deprecated 13.3 - JFactory::getApplication() will no longer attempt to retrieve an instance of JApplication if self::$application is not set.
+ * Instead, an Exception will be thrown if the object is not set.
+ */
+ JLog::add(
+ 'Using JFactory::getApplication() to instantiate applications is deprecated. JApplicationBase classes should register directly to JFactory::$application.',
@elkuku
elkuku added a note

I believe this triggers a checkstyle line length warning..

@mbabker Owner
mbabker added a note

The updated commit should fix that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@eddieajau

I'd probably go further and suggest JFactory as a whole is not something we need in the core platform anymore.

@mbabker
Owner

You could make that argument. Once you have a JApplicationWeb instance (since that most directly replaces JApplication for that purpose), you can get the config, session, document, user (identity) and language objects out of the box. Adding support for the other objects if your application needs it is too easy. It's a matter of working away from using the factory methods I guess, and not having everyone's necks on the chopping block when the decision to deprecate it is made.

@eddieajau

I'm just suggesting it's not needed in the core platform anymore. I would probably argue against dropping it from the CMS.

@mbabker
Owner

I don't disagree, I'm just saying it would take some work to break the Platform of its use, the most common probably being the getApplication and getDbo methods.

@eddieajau

Right, it's definitely a bigger job.

@davidhurley

I agree that JFactory may not be needed in the core but think it may be a bit before that is realized. I think this commit is a good fix to move towards using JApplicationBase as opposed to the legacy class.

@LouisLandry

This is great stuff @mbabker. I agree that JFactory is something that we need to think about. Going forward I want to see us using the application object for getting most of the things we used to get from JFactory and inject the application object into places where it is needed as a dependency. That will allow us a lot more flexibility and testability down the road.

@mbabker
Owner

ping

Is this something we want to slip in to 12.3 or hold off for a while longer?

@eddieajau eddieajau merged commit 3b4d0d2 into joomla:staging
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 25 additions and 5 deletions.
  1. +25 −5 libraries/joomla/factory.php
View
30 libraries/joomla/factory.php
@@ -17,7 +17,7 @@
abstract class JFactory
{
/**
- * @var JApplication
+ * @var JApplicationBase
* @since 11.1
*/
public static $application = null;
@@ -80,15 +80,15 @@
/**
* Get a application object.
*
- * Returns the global {@link JApplication} object, only creating it if it doesn't already exist.
+ * Returns the global {@link JApplicationBase} object, only creating it if it doesn't already exist.
*
* @param mixed $id A client identifier or name.
* @param array $config An optional associative array of configuration settings.
* @param string $prefix Application prefix
*
- * @return JApplication object
+ * @return JApplicationBase object
*
- * @see JApplication
+ * @see JApplicationBase
* @since 11.1
* @throws Exception
*/
@@ -96,12 +96,32 @@ public static function getApplication($id = null, array $config = array(), $pref
{
if (!self::$application)
{
+ /*
+ * @deprecated 13.3 - JFactory::getApplication() will no longer attempt to retrieve an instance of JApplication if self::$application is not set.
+ * Instead, an Exception will be thrown if the object is not set.
+ */
+ JLog::add(
+ sprintf(
+ 'Using %s to instantiate applications is deprecated. JApplicationBase classes should register directly to JFactory::$application.',
+ __METHOD__
+ ),
+ JLog::WARNING,
+ 'deprecated'
+ );
+
if (!$id)
{
throw new Exception('Application Instantiation Error', 500);
}
- self::$application = JApplication::getInstance($id, $config, $prefix);
+ if (class_exists('JApplication'))
+ {
+ self::$application = JApplication::getInstance($id, $config, $prefix);
+ }
+ else
+ {
+ throw new Exception('JApplication not loaded, unable to load JApplication instance', 500);
+ }
}
return self::$application;
Something went wrong with that request. Please try again.