Skip to content
This repository has been archived by the owner on Nov 26, 2017. It is now read-only.

Remove coupling to JFactory #32

Closed
14 tasks done
eddieajau opened this issue Mar 3, 2013 · 12 comments
Closed
14 tasks done

Remove coupling to JFactory #32

eddieajau opened this issue Mar 3, 2013 · 12 comments

Comments

@eddieajau
Copy link
Contributor

We need to remove the coupling to the old JFactory, now \Joomla\Factory.

  • Application
  • Archive
  • Client
  • Controller
  • Filesystem
  • Filter
  • Form
  • Http
  • Language
  • Log
  • Model
  • OAuth1
  • OAuth2
  • Session
@dongilbert
Copy link
Contributor

So would the basic thought on this be that in cases where the Factory::getXXXXXX method is just a wrapper for specific getInstance() (as in the case of Database\Driver) that we just drop it in favor of the getInstance method for that class?

The only downside of that approach is in the case of Database\Driver::getInstance() it requires you to pass it a config, which is handled by Factory. So, currently there is no dependency within the Driver::getInstance() method, but if we switch to doing it as described above, then we create a Config dependency.

Not sure what to do.

@eddieajau
Copy link
Contributor Author

So I think in the case of JFactory::getDbo we replace that with the database factory where we know we have the configuration for the database. If we can't guarantee that we will have configuration, then we should rely only on dependancy injection. Application handles configuration so I think we require a node and a format for the database credentials and from there we can safely support loadDatabase. Does that make sense?

eddieajau added a commit that referenced this issue Mar 9, 2013
eddieajau added a commit that referenced this issue Mar 11, 2013
Removes Factory and simplifies the package by using more DI.

Refs #32
eddieajau added a commit that referenced this issue Mar 11, 2013
Removes Factory and simplifies the package by using more DI.

Refs #32
@piotr-cz
Copy link
Contributor

piotr-cz commented Aug 3, 2013

After @mbabker 's patch, Packages using JLanguage::getInstance() like Text::_('TRANSLATE') are always falling back to default language.

There's no place where current language is stored (was Factory::$language before).
I described the problem at the Framework group but have no idea how to handle this.

@dongilbert
Copy link
Contributor

I did a search, and of the remaining methods in Factory, only these usages remain:

// Places that reference: Factory::getDbo()
docs/coding-standards/php.md
src/Joomla/Database/README.md
src/Joomla/Form/Field/Sql.php
src/Joomla/Form/Rule/Email.php
src/Joomla/Language/LanguageHelper.php
src/Joomla/Log/Logger/Database.php
src/Joomla/Session/Storage/Database.php

// Places that reference: Factory::getConfig()
src/Joomla/Form/Field/Timezone.php
src/Joomla/Form/Form.php
src/Joomla/Form/Tests/JFormTest.php
src/Joomla/Log/Logger/Formattedtext.php
src/Joomla/Session/Session.php
src/Joomla/Session/Storage/Memcache.php
src/Joomla/Session/Storage/Memcached.php

// Places that reference: Factory::getsession()
src/Joomla/Oauth1/Client.php
src/Joomla/Session/Session.php

// Places that reference: Factory::$application
src/Joomla/Oauth2/Client.php
src/Joomla/Session/_Tests/JSessionTest.php

@dongilbert
Copy link
Contributor

Oops, forgot about Factory::getStream() (it's not been removed, yet)

src/Joomla/Archive/Bzip2.php
src/Joomla/Archive/Gzip.php

@dongilbert
Copy link
Contributor

Once #238 is merged, we can check off Form.

@dongilbert
Copy link
Contributor

Once #239 is merged, we can check off Language.

@dongilbert
Copy link
Contributor

Once #241 is merged, we can check off Session.

@dongilbert
Copy link
Contributor

Once #244 is merged, we can check off Oauth 1 & 2.

@dongilbert
Copy link
Contributor

Once #242 is merged, we can check off Log. And that will be the last of them.

@piotr-cz
Copy link
Contributor

I think the solution for Text is to set the Language instance, otherwise it will always pull the default one.
(or convert it to instance and provide $language in constructor)

class Text
{
    protected static $lang;

    public static function setLanguage(Language $lang)
    {
        static::$lang = $lang;
    }

    public static function _($string, $jsSafe = false, $interpretBackSlashes = true, $script = false)
    {
        $lang = self::$lang;

        //...
    }
}

and then

// In application
$language = Language::getInstance('pl-PL');
Text::setLanguage($lang);

// Later on
echo Text::_('HELLO_WORLD');

@dongilbert
Copy link
Contributor

That's a reasonable approach, @piotr-cz

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants