[#20020] - [RFC] Introduce "Facade" layer
- Closed
- 3 Apr 2018
- Medium
- Build: staging
- # 20020
Right now in the core API, we have several components whose public interactions are entirely through static APIs (filesystem, logging, and the JText class being three prominent examples). Moving these to object oriented structures is difficult because even removing the static keyword from the method declaration creates a B/C break and one that's not the most friendly to work around. Likewise, these static method calls cannot be mocked in testing environments meaning proper unit testing of objects calling those systems cannot be done, and in a lot of cases this mandates that a unit test class sets up mocks of various global services.
One way we can cope with this is by introducing something akin to Laravel's Facade class, which is in essence a fancy way of proxying static method calls to an object resolved from the DI container. It would basically allow us to keep the existing classes with their static APIs in place (so the existing classes would have to remain as the Facade classes), but the actual code could be migrated to new PHP classes which are designed in an object oriented structure. Additionally, Laravel's Facade class exposes some API making them configurable in a testing environment, which would allow these static calls to be configured to use a mock object versus the production code.
joomla-cms-bot
- | Labels |
Added:
?
?
|
||
franz-wohlkoenig
- | Category | ⇒ | Feature Request |
Not quite. It'd be something like this.
class JText extends Facade
{
// API to tell the Facade that __callStatic will resolve to our new Translator class goes in here
}
class Translator
{
protected $language;
public function __construct(JLanguage $language)
{
$this->language = $language;
}
public function _($string, $jsSafe = false, $interpretBackSlashes = true, $script = false)
{
/*
* Right now static JText::_() has a dependency to JFactory::getLanguage()
* now the OOP class gets this dependency injected, removes a hardcoded
* global dependency and actually enables the translator to support translating
* in multiple languages versus only being able to use the JLanguage instance
* in the global factory. API win!
*/
}
}So all the JText::_() calls would just keep working without changing a line of code where the class is in use, because JText will turn into a class extending this Facade accessor and the accessor's __callStatic method handles resolving to the correct object instance. Programmatically, it's as ugly and controversial as JFactory::getContainer()->get('translator'); but from a practical perspective it opens the door to break a very hard dependency on explicit static methods in classes.
What do you say to rewrite the JText :: _ () method to use a code like:
class Text {
...
public static function _($string, ...) {
$translator = Factory::getContainer()->get('translator');
return $translator->_($string, ...);
}
...
}
You can do that, but it still doesn't address the issue of a hardcoded and non-mockable static method call. The advantage (and controversial part of Laravel's facades at least) here is that you can explicitly configure the facade internals to point to a mock or a test service. So yes, it's still a static call to JText::_(), but it's configurable/mockable.
Assuming this production code:
class JText extends Facade {}
class Translator
{
private $language;
public function __construct(JLanguage $language)
{
$this->language = $language;
}
public function _($string, $jsSafe = false, $interpretBackSlashes = true, $script = false)
{
return $this->language->_($string);
}
}
class TranslationExtension extends Twig_Extension
{
public function getFunctions()
{
return [
new Twig_Function('translate', [$this, 'translate']),
];
}
public function translate($string, $jsSafe = false, $interpretBackSlashes = true, $script = false)
{
return JText::_($string, $jsSafe, $interpretBackSlashes, $script);
}
}As Joomla is set up today, to write a unit test for my TranslationExtension class, I loosely need something like this:
class TranslationExtensionText extends TestCase
{
protected function setUp()
{
parent::setUp();
$this->saveFactoryState();
JFactory::$language = $this->getMockLanguage();
}
protected function tearDown()
{
$this->restoreFactoryState();
parent::tearDown();
}
public function testTranslate()
{
// Test
}
}With your proposal, I need something like this:
class TranslationExtensionText extends TestCase
{
protected function setUp()
{
parent::setUp();
$this->saveFactoryState();
$translator = $this->createMock(Translator::class);
$container = new Joomla\DI\Container;
$container->set('translator', $translator);
JFactory::$container = $container;
}
protected function tearDown()
{
$this->restoreFactoryState();
parent::tearDown();
}
public function testTranslate()
{
// Test
}
}With a Facade layer similar to Laravel's, it become's something like this (based on https://laravel.com/docs/5.6/mocking#mocking-facades)
class TranslationExtensionText extends TestCase
{
public function testTranslate()
{
/*
* This set of instructions fully creates the mock assertions
* $mock = $this->createMock(Translator::class);
* $mock->expects($this->once())->method('_')->with('COM_TRANSLATE')->willReturn('Translate');
*/
JText::shouldReceive('_')
->once()
->with('COM_TRANSLATE')
->andReturn('Translate');
// Test
}
}So the key thing here is the classes holding the static APIs are extending from a somewhat black hat magical class where you can mock the underlying service provider. Whereas today, with the current static APIs, to test something which uses them you have to run that production code (meaning you cannot mock what JText::_() does, the point of this proposal is you can build an accessor/manager type base class that "service" classes like JLog or JText would extend and make use of PHP's magic methods and a well defined configuration API to intercept the static calls and mock it the same way you can create mocks of non-static class methods).
| Status | New | ⇒ | Discussion |
Actually, nevermind. I don't have time to implement another major architectural change in core, and I feel like I'd be the only one to work on the code to see this through, and I've already spent over a year and a half working on prepared statement support with minimal support other than "go for it" and some likes and retweets when asking for help.
| Status | Discussion | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2018-04-03 12:03:28 |
| Closed_By | ⇒ | mbabker |
Do I understand it correctly? You want to replace all occurrences of (external static methods), ex:
JLog::add(...)to something likeFacade::getClass('JLog')::add(...);.