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

Feature/phpunit via strategy #100

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

koseduhemak
Copy link
Contributor

fix #26 .
Added PhpunitStrategy to cope with problems while unit testing zend applications. Updated README also with example code.
Sadly we need a special PHPUNIT variable set in phpunit.xml or boostrap.php file of the unit tests:
SLMLOCALE_DISABLE_STRATEGIES has to be set to true. Otherwise we cannot differentiate between application was run by console command or phpunit command.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 82.051% when pulling 4398922 on koseduhemak:feature/phpunit-via-strategy into 5c272ac on basz:develop.

@koseduhemak
Copy link
Contributor Author

added more test cases

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 82.564% when pulling b37d752 on koseduhemak:feature/phpunit-via-strategy into 5c272ac on basz:develop.

@svycka
Copy link
Collaborator

svycka commented Dec 13, 2017

just wondering @koseduhemak why this redirect happens? UriPath strategy should not do redirect if locale is set in path is this a bug if so maybe better fix it?

@koseduhemak
Copy link
Contributor Author

I digged a little more into it.
The problem seems to be like the following:
If we run a phpunit test, a console request is made at the moment phpunit starts a test case. The request object is then filled with data:

Zend\Console\Request Object
(
    [params:protected] => Zend\Stdlib\Parameters Object
        (
            [storage:ArrayObject:private] => Array
                (
                    [0] => --testsuite
                    [1] => user-registration
                )

        )

    [envParams:protected] => Zend\Stdlib\Parameters Object
        (
            [storage:ArrayObject:private] => Array
                (
                )

        )

    [scriptName:protected] => /Users/myuser/dev/myproject/vendor/bin/phpunit
    [metadata:protected] => Array
        (
        )

    [content:protected] => Array
        (
            [0] => --testsuite
            [1] => user-registration
        )

)

Because the Zend MVC Application is initialized in the setUp() method of phpunit tests extending AbstractHttpControllerTestCase, SlmLocale tries to extract the language from the above request object. Because it is of class Zend\Console\Request, it has no uri to parse the locale from. Therefore it "saves/caches" the result (no locale found => redirect to /en) and will now short circuit every new dispatch to /en.
This is due the fact, that SlmLocale attaches to MvcEvent::EVENT_ROUTE event in Module.php. So, if dispatch within the test case like so:

        $this->dispatch('/en/myuri');
        $this->assertResponseStatusCode(200);

The uri does not get processed by strategies of SlmLocale anymore, because it previously processed the Zend\Console\Request and short-circuit every dispatch to /en. Of course, this leads to status code 302 instead of 200.

So the only possibility I see here is to modify Detector.php and add a check in method detect like:

if ($isConsoleRequest === true && $scriptWasExecutedByPhpunit === true) {
    // skip processing and just return / do not modify anything just let the application process everything and act like SlmLocale is not installed
}

I hope I understood it correctly and could explain it clearly and not to misleading.

@koseduhemak
Copy link
Contributor Author

koseduhemak commented Dec 13, 2017

But even if we modify Detector.php like I stated in my previous comment, we would get 404 instead of 200, because uris like /en/myuri cannot be matched anymore. A RouteMatch can only be retrieved, if /myuri is dispatched (as /en is not configured as route segment within the module.config.php's router config).

@koseduhemak
Copy link
Contributor Author

As an solution we could maybe ditch Detector.php and write a new class for HttpRouting.
I found another github project which does exactly that: https://github.com/xelax90/zf2-language-route

The custom LanguageTreeRouteStack could be something like:

<?php

/*
 * Copyright (C) 2015 schurix
 *
 * This program is free software; you can redistribute it and/or
 * modify it under the terms of the GNU General Public License
 * as published by the Free Software Foundation; either version 2
 * of the License, or (at your option) any later version.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License
 * along with this program; if not, write to the Free Software
 * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.
 */

namespace Base\Mvc\Router\Http;

use Base\Options\LanguageRouteOptions;
use User\Entity\LocaleUserInterface;
use Zend\Authentication\AuthenticationServiceInterface;
use Zend\Http\Response;
use Zend\I18n\Translator\TranslatorInterface;
use Zend\Mvc\Router\Http\TranslatorAwareTreeRouteStack;
use Zend\Mvc\Router\RouteMatch;
use Zend\Session\Container;
use Zend\Stdlib\RequestInterface;
use Zend\Uri\Http;
use Zend\Uri\Uri;

/**
 * Manages multilanguage routes by adding a language key to the baseUrl
 *
 * @author schurix
 */
class LanguageTreeRouteStack extends TranslatorAwareTreeRouteStack
{

    /** @var LanguageRouteOptions */
    protected $languageOptions;

    /** @var AuthenticationServiceInterface */
    protected $authenticationService;

    /** @var string */
    protected $lastMatchedLocale;

    /** @var  Uri */
    protected $redirect;

    function getLanguageOptions()
    {
        return $this->languageOptions;
    }

    function setLanguageOptions(LanguageRouteOptions $languageOptions)
    {
        $this->languageOptions = $languageOptions;
    }

    function getAuthenticationService()
    {
        return $this->authenticationService;
    }

    function setAuthenticationService(AuthenticationServiceInterface $authenticationService)
    {
        $this->authenticationService = $authenticationService;
    }

    /**
     * Returns the locale that was found in the last matched URL. It is also
     * stored if no RouteMatch instance is provided (e.g. 404 error)
     * @return string
     */
    function getLastMatchedLocale()
    {
        return $this->lastMatchedLocale;
    }

    /**
     * assemble(): defined by \Zend\Mvc\Router\RouteInterface interface.
     *
     * @see    \Zend\Mvc\Router\RouteInterface::assemble()
     * @param  array $params
     * @param  array $options
     * @return mixed
     * @throws Exception\InvalidArgumentException
     * @throws Exception\RuntimeException
     */
    public function assemble(array $params = array(), array $options = array())
    {
        // Assuming, this stack can only orrur on top level
        // TODO is there any way to ensure that this is called only for top level?

        // get translator
        $translator = null;
        if (isset($options['translator'])) {
            $translator = $options['translator'];
        } elseif ($this->hasTranslator() && $this->isTranslatorEnabled()) {
            $translator = $this->getTranslator();
        }

        $languages = $this->getRouteLanguages();

        $oldBase = $this->baseUrl; // save old baseUrl
        // only add language key when more than one language is supported
        if (count($languages) > 1) {
            if (isset($params['locale'])) {
                // use parameter if provided
                $locale = $params['locale'];
                // get key for locale
                $key = array_search($locale, $languages);
            } elseif (is_callable(array($translator, 'getLocale'))) {
                // use getLocale if possible
                $locale = $translator->getLocale();
                // get key for locale
                $key = array_search($locale, $languages);
            }

            if (!empty($key)) {
                // add key to baseUrl
                $this->setBaseUrl($oldBase . '/' . $key);
            }
        }

        $res = parent::assemble($params, $options);
        // restore baseUrl
        $this->setBaseUrl($oldBase);
        return $res;
    }

    /**
     * match(): defined by \Zend\Mvc\Router\RouteInterface
     *
     * @see    \Zend\Mvc\Router\RouteInterface::match()
     * @param  Request $request
     * @param  integer|null $pathOffset
     * @param  array $options
     * @return RouteMatch|null
     */
    public function match(RequestInterface $request, $pathOffset = null, array $options = array())
    {
        // Languages should only be added on top level. Since there seems to be
        // no way to ensure this stack is only at top level, the language has
        // top be checked every time this method is called.
        /* if($pathOffset !== null){
            return parent::match($request, $pathOffset, $options);
        } */

        if (!method_exists($request, 'getUri')) {
            return null;
        }

        if ($this->baseUrl === null && method_exists($request, 'getBaseUrl')) {
            $this->setBaseUrl($request->getBaseUrl());
        }

        /* @var $translator TranslatorInterface */
        $translator = null;
        if (isset($options['translator'])) {
            $translator = $options['translator'];
        } elseif ($this->hasTranslator() && $this->isTranslatorEnabled()) {
            $translator = $this->getTranslator();
        }

        $languages = $this->getRouteLanguages();
        $languageKeys = array_keys($languages);

        // save old baseUrl
        $oldBase = $this->baseUrl;
        $locale = null;

        // extract /-separated path parts
        $uri = $request->getUri();
        $baseUrlLength = strlen($this->baseUrl);
        $path = ltrim(substr($uri->getPath(), $baseUrlLength), '/');
        $pathParts = explode('/', $path);

        // check if language was provided in first part
        if (count($languages) > 1 && in_array($pathParts[0], $languageKeys)) {
            // if language was provided, save the locale and adjust the baseUrl
            $locale = $languages[$pathParts[0]];
            $this->setBaseUrl($oldBase . '/' . $pathParts[0]);
        } else {
            if (!empty($this->getAuthenticationService()) && $this->getAuthenticationService()->hasIdentity()) {
                // try to get user language if no language was provided by url
                $user = $this->getAuthenticationService()->getIdentity();
                if ($user instanceof LocaleUserInterface) {
                    $userLocale = $user->getLocale();
                    if (in_array($userLocale, $languages)) {
                        $locale = $userLocale;
                    }
                }
            }

            // forcing language if attached query param
            if (method_exists($request, "getQuery")) {
                $queryLocale = $request->getQuery('lang'); // new language

                if (in_array($queryLocale, $languages)) {
                    $locale = $queryLocale;
                }
            }

            if (empty($locale)) {
                $session = new Container('base');

                if ($session->offsetExists('lang')) {
                    $sessionLocale = $session->offsetGet('lang'); // current language
                    if (in_array($sessionLocale, $languages)) {
                        $locale = $sessionLocale;
                    }
                }

                #if (empty($locale) && !empty($translator) && is_callable(array($translator, 'getLocale'))) {
                // If stil no language found, check the translator locale
                #$locale = $translator->getLocale();
                #}
            }

            if (empty($locale)) {
                $locale = $this->getLanguageOptions()->getDefaultLocale();
            }
        }

        // set the last matched locale
        $this->lastMatchedLocale = $locale;

        $res = parent::match($request, $pathOffset, $options);
        $this->setBaseUrl($oldBase);
        if ($res instanceof RouteMatch && !empty($locale)) {
            $res->setParam('locale', $locale);
        }

        return $res;
    }

    protected function getRouteLanguages()
    {
        if (!empty($this->getLanguageOptions())) {
            return $this->getLanguageOptions()->getLanguages();
        }
        return [];
    }

    public function getRedirect()
    {
        return $this->redirect;
    }

}

We then could modify the match() method and add the logic of processing the different strategies defined by SlmLocale there. But instead of returning a response object, we just return the route match. That way it should be possible to solve the problem.
But this seems to be a huge amount of work as we need to completely refactor SlmLocale...

@svycka
Copy link
Collaborator

svycka commented Dec 13, 2017

hmm do we have problem with console routes or that AbstractHttpControllerTestCase uses zend\console\request as http request? I have no clue how AbstractHttpControllerTestCase works but guess will have to take a look at it :)

@svycka
Copy link
Collaborator

svycka commented Dec 13, 2017

okay, I think found the problem. We try to detect languages on bootstrap phase but url path is set after bootstrap so maybe moving this to route phase would solve the problem.

something like this in SlmLocale\Module.php

    public function onBootstrap(EventInterface $e)
    {
        $app = $e->getApplication();
        $sm = $app->getServiceManager();
        $detector = $sm->get(Detector::class);

        $em = $app->getEventManager();
        $em->attach(MvcEvent::EVENT_ROUTE, function ($e) use ($app, $detector) {
            $result = $detector->detect($app->getRequest(), $app->getResponse());
            if ($result instanceof ResponseInterface) {
                return $result;
            } else {
                Locale::setDefault($result);
            }
        }, PHP_INT_MAX);
    }

unfotunetly I don't have project to test this @koseduhemak maybe you can? I am not sure this wont break something else

@koseduhemak
Copy link
Contributor Author

koseduhemak commented Dec 13, 2017

@svycka I tried your suggestion and replaced the onBootstrap method in Module.php of SlmLocale with your new code. This way phpunit tests start to work again, but sadly we can not retrieve the locale within another onBootstrap function in Application module to f.e. set the locale in MvcTranslator / set the locale in ViewHelpers / set the locale in DoctrineTranslatable etc. as the locale is detected too late.

Module.php in Application module:

 public function onBootstrap(MvcEvent $e)
    {
        // <-- Language Settings -->
        $lang = null;
        $eventManager = $e->getApplication()->getEventManager();
        $serviceManager = $e->getApplication()->getServiceManager();
        $moduleRouteListener = new ModuleRouteListener();
        $moduleRouteListener->attach($eventManager);

        $lang = \Locale::getDefault(); // the locale is not detected, this returns the default locale every time

        $translator = $serviceManager->get('MvcTranslator');

        $translator
            ->setLocale($lang)
            ->setFallbackLocale('de_DE');
    ...
}

@svycka
Copy link
Collaborator

svycka commented Dec 14, 2017

okay, that's a problem. Of course, you could attach MvcTranslator setLocale also in route phase after slmlocale. But changing this is BC break and not sure about this.

So I am not sure about this @basz what do you think?

maybe we could create DetectLanguageListener and do not attach to any event at all but leave this for the user and he will decide where to put it under route event or could add as before in onBootstrap or something else. But I am not sure about this.

Or maybe we could fire a new event to tell application about found and set language? This way would be clear where and how to get detected language.

@basz
Copy link
Owner

basz commented Dec 14, 2017

would an additional EnviromentVariableStrategy (LOCALE=en phpunit) be useful?

I'm sorry, I haven't looked enough to really understand this issue at hand. I'm not using it actively anymore so perhaps this is a silly suggestion.

@koseduhemak
Copy link
Contributor Author

Hi guys... Seems like we have to code our own module. My team decided to code a module by ourselves which is more suited to our needs. Therefore I am not able to investigate this (and other issues) further...
However, we based our module on some of your ideas (mainly the one with the different strategies). If you like you can check it out here: https://github.com/koseduhemak/zf3-locale-router

We did not release a stable version, as we are not finished with initial coding / testing yet, but we made good progress so far.

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.

None yet

4 participants