Fix issue #67 - New AssetStrategy #93

koseduhemak · 2017-11-07T11:47:23Z

Fixing problems of issue #67, which describes problems by using SlmLocale in combination with the famous https://github.com/RWOverdijk/AssetManager module.

Problem

When using asset manager to manage assets in your project, uris of assets getting rewritten to locale aware uris. This leads to 404 not found, because the assetmanager is not able to resolve them anymore. Therefore we need a strategy to cope such uris.

Solution

The provided AssetStrategy checks if the requested uri is an asset and prevents url rewriting through other strategies.

Example config

return [
    'slm_locale' => [
        'default' => 'de_DE',

        'supported' => ['en_GB', 'de_DE'],

        'strategies' => [
           [
                'name' => SlmLocale\Strategy\AssetStrategy::class,
                'options' => [
                    'file_extensions' => [
                        'css', 'js'
                    ]
                ]
            ],
            'query',
            [
                'name' => \SlmLocale\Strategy\UriPathStrategy::class,
                'options' => [
                    'redirect_when_found' => true,
                    'aliases' => array('de' => 'de_DE', 'en' => 'en_GB'),
                ]
            ],
            'cookie',
            'acceptlanguage'
        ],

        'mappings' => [
            'en' => 'en_GB',
            'de' => 'de_DE',
        ]
    ],
];

koseduhemak · 2017-11-07T16:36:03Z

Why is travis failing? I am not experienced in pull requests... Please provide some hint so I can fix...

basz · 2017-11-07T17:06:15Z

Cause you have code style errors.

See log: https://travis-ci.org/basz/SlmLocale/jobs/298633316

You are able to run those checks yourself before committing with this command

vendor/bin/php-cs-fixer fix -v --diff --dry-run

basz · 2017-11-07T20:47:12Z

One of your tests isnt passing

koseduhemak · 2017-11-08T08:29:54Z

By fixing code style issues I accidentally removed a line of code... Now everything should be working.

coveralls · 2017-11-08T08:39:39Z

Coverage increased (+0.4%) to 80.211% when pulling 85f13fd on koseduhemak:master into 7df75eb on basz:master.

svycka

looks good feature but would be good if you could update documentation or at least add example

svycka · 2017-11-08T09:26:17Z

src/SlmLocale/Strategy/AssetStrategy.php

+    /** @var  array */
+    protected $file_extensions;
+
+    public function detect(LocaleEvent $event)


this method is can be removed already exists in AbstractStrategy

svycka · 2017-11-08T09:33:05Z

src/SlmLocale/Strategy/AssetStrategy.php

+
+    public function setOptions(array $options = [])
+    {
+        if (array_key_exists('file_extensions', $options)) {


should not be optional or at least default should be an empty array

default value is now an empty array.

svycka · 2017-11-08T09:34:36Z

src/SlmLocale/Strategy/AssetStrategy.php

+        }
+    }
+
+    public function setOptions(array $options = [])


@basz I am not sure but this method should be in StrategyInterface why it is not there?

honestly no idea. I see multiple methods in https://github.com/basz/SlmLocale/blob/master/src/SlmLocale/Strategy/AbstractStrategy.php that should be define d on the interface. Perhaps in a different PR?

svycka · 2017-11-08T09:36:18Z

tests/SlmLocaleTest/Strategy/AssetStrategyTest.php

+<?php
+/**
+ * Created by PhpStorm.
+ * User: mfuesslin


remove this doc block

svycka · 2017-11-08T09:48:46Z

src/SlmLocale/Locale/Detector.php

@@ -191,6 +191,9 @@ public function detect(RequestInterface $request, ResponseInterface $response =
            $events->triggerEventUntil(function ($r) use (&$return) {
                if ($r instanceof ResponseInterface) {
                    $return = true;
+                } elseif ($r === false) {
+                    // return true, if a previous listener returned false. So we can stop processing further listeners. This is needed for AssetStrategy to prevent redirects.
+                    return true;


would be nice to have this tested also

I do not know exactly how to test this specific part. Maybe someone can help me here?

svycka · 2017-11-08T09:51:04Z

src/SlmLocale/Strategy/AssetStrategy.php

+ */
+class AssetStrategy extends AbstractStrategy
+{
+    /** @var  array */


maybe add description something like a list of extensions to be ignored or example

added example in code comment and documentation.

svycka · 2017-11-08T09:52:37Z

src/SlmLocale/Strategy/AssetStrategy.php

+class AssetStrategy extends AbstractStrategy
+{
+    /** @var  array */
+    protected $file_extensions;


default to empty array []

basz

good addition. please fix the suggestions from svycka and I think it is good to go. thanks

basz · 2017-11-08T10:16:12Z

src/SlmLocale/Strategy/AssetStrategy.php

+        }
+    }
+
+    public function setOptions(array $options = [])


honestly no idea. I see multiple methods in https://github.com/basz/SlmLocale/blob/master/src/SlmLocale/Strategy/AbstractStrategy.php that should be define d on the interface. Perhaps in a different PR?

coveralls · 2017-11-08T14:41:58Z

Coverage increased (+0.6%) to 80.423% when pulling ed76a86 on koseduhemak:master into 7df75eb on basz:master.

svycka · 2017-11-16T13:26:13Z

refactored here #97

Maximilian Füsslin added 6 commits November 7, 2017 11:37

created AssetStrategy to prevent uris of assets from being rewritten

ea3fafc

updated AssetStrategy

4d309a2

updated comments

a98eb27

updated comments

b49fbe4

fixed bug in AssetStrategy

c7ff1df

created test case for asset strategy

586fb05

fixed code style errors

4b2ef49

fixed bug in AssetStrategy

85f13fd

svycka requested changes Nov 8, 2017

View reviewed changes

svycka requested a review from basz November 8, 2017 09:57

basz approved these changes Nov 8, 2017

View reviewed changes

Maximilian Füsslin added 2 commits November 8, 2017 12:56

updated with the tips of @svycka at #93

2877ffa

updated documentation with examples for AssetStrategy

ed76a86

svycka self-assigned this Nov 15, 2017

svycka added this to the v0.3.0 milestone Nov 15, 2017

svycka pushed a commit that referenced this pull request Nov 16, 2017

updated with the tips of @svycka at #93

2b17f0d

svycka mentioned this pull request Nov 16, 2017

Fix issue #67 - New AssetStrategy #97

Merged

svycka closed this Nov 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue #67 - New AssetStrategy #93

Fix issue #67 - New AssetStrategy #93

koseduhemak commented Nov 7, 2017

koseduhemak commented Nov 7, 2017

basz commented Nov 7, 2017

basz commented Nov 7, 2017

koseduhemak commented Nov 8, 2017

coveralls commented Nov 8, 2017

svycka left a comment

svycka Nov 8, 2017

svycka Nov 8, 2017

koseduhemak Nov 8, 2017

svycka Nov 8, 2017

basz Nov 8, 2017

svycka Nov 8, 2017

svycka Nov 8, 2017

koseduhemak Nov 8, 2017

svycka Nov 8, 2017

koseduhemak Nov 8, 2017

svycka Nov 8, 2017

basz left a comment

basz Nov 8, 2017

coveralls commented Nov 8, 2017

svycka commented Nov 16, 2017

Fix issue #67 - New AssetStrategy #93

Fix issue #67 - New AssetStrategy #93

Conversation

koseduhemak commented Nov 7, 2017

Problem

Solution

Example config

koseduhemak commented Nov 7, 2017

basz commented Nov 7, 2017

basz commented Nov 7, 2017

koseduhemak commented Nov 8, 2017

coveralls commented Nov 8, 2017

svycka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 8, 2017

svycka commented Nov 16, 2017