first drafts of theme switcher module #158

ghost · 2015-10-16T12:10:29Z

Theme switcher is working now, but you have to manually refresh the page to change th text.

TODO:

~~unit tests~~
~~Fix some code bugs, better cohesion and coupling~~
~~Move getConfig() logic to a factory class~~
~~Add styles for all themes~~
~~Simplify~~
~~Merge both modules into one~~

samsonasik · 2015-10-16T12:14:20Z

config/application.config.php

@@ -22,6 +23,7 @@
        'LearnZF2Authentication',
        'LearnZF2Captcha',
        'LearnZF2AjaxImageGallery',
+        'LearnZF2Themes',


require two modules ? Themes and LearnZF2Themes ?

Yes. Still thinking how to make it simple. Got any suggestions?

only LearnZF2Themes

…in controller :X

ghost · 2015-10-20T11:35:38Z

@samsonasik @acelaya Would you mind doing a review?

samsonasik · 2015-10-20T11:44:33Z

module/LearnZF2Themes/themes/blue-theme/view/learn-zf2-themes/index/index.phtml

+        }
+?>
+        </div>
+        <p>This is where all the magic happens. The system retrives the theme name from the Config


s/retrives/retrieves

ghost · 2015-10-20T18:15:37Z

Sometimes I'm amazed by my stupidity and the code I write...

samsonasik · 2015-10-21T03:14:33Z

module/LearnZF2Themes/themes/blue-theme/view/learn-zf2-themes/index/index.phtml

+            $headScript->prependFile($publicDir.$file);
+        }
+
+        return new self();


update this as well as code updated.

samsonasik · 2015-10-21T08:43:38Z

module/LearnZF2Themes/src/LearnZF2Themes/Factory/ReloadConfigFactory.php

+            $serviceLocator->setAllowOverride(false);
+        }
+
+        return $serviceLocator;


imo, the zf2 factory should create another instance that may pull service from serviceLocator, not a serviceLocator instance itself, @acelaya do you have some opinion about it ? I think it can be replaced by SharedEventManager, in Module class:

use LearnZF2Themes\Service\ReloadService; $shm->attach(ReloadService::class, 'reload', function($e) use ($this->service) { $reloadService = $e->getTarget(); // do the re-set config here..., or use attachAggregate with passed listener \ // that handle it $reloadService->getEventManager() ->attachAggregate($this->service->get('ConfigChangerListener')); });

in ConfigChangerListener, we can do:

and in the LearnZF2Themes\Service\ReloadService , we can do :

namespace LearnZF2Themes\Service; use Zend\EventManager\EventManager; use Zend\EventManager\EventManagerAwareInterface; use Zend\EventManager\EventManagerInterface; use Zend\EventManager\EventManagerAwareTrait; class ReloadService implements EventManagerAwareInterface { use EventManagerAwareTrait; public function reload() { $this->getEventManager()->trigger(__FUNCTION__, $this); } }

So, in controller, we can do:

$this->reloadService->reload();

more clear imo ^^

Sorry, I'm a little bit disconnected on this subject. I'll take a look at
it this afternoon

Alejandro Celaya Alastrué
El 21/10/2015 10:43, "Abdul Malik Ikhsan" notifications@github.com
escribió:

In
module/LearnZF2Themes/src/LearnZF2Themes/Factory/ReloadConfigFactory.php
#158 (comment):

\* {@inheritdoc}

*/

public function __invoke(ServiceLocatorInterface $serviceLocator)

{

$request = $serviceLocator->get('Request');

if ($request->isPost()) {

$config = $serviceLocator->get('Config');

$themeName = $request->getPost()['themeName'];

$serviceLocator->setAllowOverride(true);

$config['theme']['name'] = $themeName;

$serviceLocator->setService('Config', $config);

$serviceLocator->setAllowOverride(false);

}

return $serviceLocator;

imo, the zf2 factory should create another instance that may pull service
from serviceLocator, not a serviceLocator instance itself, @acelaya
https://github.com/acelaya do you have some opinion about it ? I think
it can be replaced by SharedEventManager, in Module class:

use LearnZF2Themes\Service\ReloadService;$shm->attach(ReloadService::class, 'reload', function($e) use ($this->service) { $reloadService = $e->getTarget(); // do the re-set config here..., or use attachAggregate with passed listener \ // that handle it $reloadService->getEventManager() ->attachAggregate($this->service->get('ConfigChangerListener'));});

in ConfigChangerListener, we can do:

and in the LearnZF2Themes\Service\ReloadService , we can do :

namespace LearnZF2Themes\Service;use Zend\EventManager\EventManager;use Zend\EventManager\EventManagerAwareInterface;use Zend\EventManager\EventManagerInterface;use Zend\EventManager\EventManagerAwareTrait;class ReloadService implements EventManagerAwareInterface { use EventManagerAwareTrait; public function reload() { $this->getEventManager()->trigger(FUNCTION, $this); }}

So, in controller, we can do:

$this->reloadService->reload();

more clear imo ^^

—
Reply to this email directly or view it on GitHub
https://github.com/sitrunlab/LearnZF2/pull/158/files#r42596990.

it's ok, thank you ;)

This could actually work. Gonna try and see. Thanks!

ghost · 2015-10-21T10:27:32Z

Bitbucket is down and travis has failed installing PhantomJS.

samsonasik · 2015-10-21T10:55:15Z

usually won't take long, hopefully on your next commit, the bitbucket service back again ;)

Warm regards,

Abdul Malik Ikhsan

Pada 21 Okt 2015, pukul 17.27, Stanimir Dimitrov notifications@github.com menulis:

Bitbucket is down and travis has failed installing PhantomJS.

—
Reply to this email directly or view it on GitHub.

samsonasik · 2015-10-21T12:49:46Z

module/LearnZF2Themes/Module.php

+    /**
+     * @var \Zend\getServiceManager\ServiceManager
+     */
+    private $service = null;


no need to assign to null here.

I like having default values, althought null is not necessery, it's more clear that the property is null by default. Odd habit.

samsonasik · 2015-10-21T13:51:34Z

module/LearnZF2Themes/Module.php

+        $app = $event->getApplication();
+        $this->service = $app->getServiceManager();
+        $eventManager = $app->getEventManager();
+        $sharedEventManager = $app->getEventManager()->getSharedManager();


can be shortcut-ed with:

$sharedEventManager = $eventManager->getSharedManager();

ghost · 2015-10-22T12:48:11Z

I implemented the module in my CMS and it took me 5 mins for the whole process. There the module loads for ~300ms. Nothing more to add. Ready for merge in no one has anything to say.

samsonasik · 2015-10-22T13:27:00Z

@acelaya could you do final review before it got merged?

acelaya · 2015-10-22T13:28:31Z

I'm not probably going to be able to review it until weekend.
Is it ok?

Alejandro Celaya Alastrué
El 22/10/2015 15:27, "Abdul Malik Ikhsan" notifications@github.com
escribió:

@acelaya https://github.com/acelaya could you do final review before it
got merged?

—
Reply to this email directly or view it on GitHub
#158 (comment).

samsonasik · 2015-10-22T13:36:05Z

Ok ;), thank you ;)

Warm regards,

Abdul Malik Ikhsan

Pada 22 Okt 2015, pukul 20.28, Alejandro Celaya notifications@github.com menulis:

I'm not probably going to be able to review it until weekend.
Is it ok?

Alejandro Celaya Alastrué
El 22/10/2015 15:27, "Abdul Malik Ikhsan" notifications@github.com
escribió:

@acelaya https://github.com/acelaya could you do final review before it
got merged?

—
Reply to this email directly or view it on GitHub
#158 (comment).

—
Reply to this email directly or view it on GitHub.

samsonasik · 2015-10-22T18:08:07Z

@stanimirdim92 you're missing migration command to add this module to sql that will be generated in data/DoctrineORMModule/Migrations

add migrations class

samsonasik · 2015-10-26T16:02:35Z

data/DoctrineORMModule/Migrations/Version20151023012800.php

+     */
+    public function up(Schema $schema)
+    {
+        // this up() migration is auto-generated, please modify it to your needs


need sql update here, I will do during merge

Forward port #158 * stanimir/theme-switcher: (25 commits) #158 LearnZF2Themes sql add migrations class module fix feedback fixes styleci............... forgot the new unit tests... stylceci stylceci stylceci added custom event listener fixed tutorial code removed phpunit phar styleci fixes, again feedback from samsonasik added code in index.php styleci added styles, fixed theme reloading, althought it sucks with that SM in controller :X added styles, fixed theme reloading, althought it sucks with that SM in controller :X styleci fixes, config paths fixes, added 100% code coverage. styleci fixes, config paths fixes, added 100% code coverage. ...

* stanimir/theme-switcher: (25 commits) #158 LearnZF2Themes sql add migrations class module fix feedback fixes styleci............... forgot the new unit tests... stylceci stylceci stylceci added custom event listener fixed tutorial code removed phpunit phar styleci fixes, again feedback from samsonasik added code in index.php styleci added styles, fixed theme reloading, althought it sucks with that SM in controller :X added styles, fixed theme reloading, althought it sucks with that SM in controller :X styleci fixes, config paths fixes, added 100% code coverage. styleci fixes, config paths fixes, added 100% code coverage. ...

samsonasik · 2015-10-26T16:18:58Z

merged after rebase at db4af03 ;)

samsonasik · 2015-10-27T16:38:02Z

@stanimirdim92 the module is life now http://learnzf2.sitrun-tech.com/learn-zf2-themes , thanks ;)
@acelaya if you have a chance to take a look, and found any issue, feel free to PR ;), thanks ;)

first drafts of theme switcher module

28e62be

samsonasik reviewed Oct 16, 2015
View reviewed changes

stanimirdim92 added 9 commits October 16, 2015 15:40

removed Themes module, cs fixes

7207ea2

styleci

e066ad4

code moved to factories, made it dry

c0af208

code moved to factories, made it dry

1f013f8

styleci fixes, config paths fixes, added 100% code coverage.

9408f41

styleci fixes, config paths fixes, added 100% code coverage.

bd88d06

added styles, fixed theme reloading, althought it sucks with that SM …

f07df81

…in controller :X

added styles, fixed theme reloading, althought it sucks with that SM …

c555fd8

…in controller :X

styleci

609962d

added code in index.php

8f1a95a

samsonasik reviewed Oct 20, 2015
View reviewed changes

stanimirdim92 added 2 commits October 20, 2015 21:10

feedback from samsonasik

e553523

styleci fixes, again

18c9a65

removed phpunit phar

6321610

samsonasik reviewed Oct 21, 2015
View reviewed changes

fixed tutorial code

db34e31

samsonasik reviewed Oct 21, 2015
View reviewed changes

stanimirdim92 added 6 commits October 21, 2015 15:57

added custom event listener

06b8202

stylceci

6671c22

stylceci

454d143

stylceci

54a0d14

forgot the new unit tests...

cc08028

styleci...............

fcfe06f

samsonasik reviewed Oct 21, 2015
View reviewed changes

stanimirdim92 added 2 commits October 22, 2015 11:37

feedback fixes

aedbf4b

module fix

ac1b38c

samsonasik and others added 2 commits October 23, 2015 01:28

add migrations class

66c76c6

Merge pull request #2 from samsonasik/stanimir/theme-switcher

778f4d7

add migrations class

samsonasik reviewed Oct 26, 2015
View reviewed changes

samsonasik added a commit that referenced this pull request Oct 26, 2015

#158 LearnZF2Themes sql

f65d0e2

samsonasik closed this Oct 26, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

first drafts of theme switcher module #158

first drafts of theme switcher module #158

ghost commented Oct 16, 2015

samsonasik Oct 16, 2015

ghost Oct 16, 2015

samsonasik Oct 16, 2015

ghost commented Oct 20, 2015

samsonasik Oct 20, 2015

ghost commented Oct 20, 2015

samsonasik Oct 21, 2015

samsonasik Oct 21, 2015

acelaya Oct 21, 2015

samsonasik Oct 21, 2015

ghost Oct 21, 2015

ghost commented Oct 21, 2015

samsonasik commented Oct 21, 2015

samsonasik Oct 21, 2015

ghost Oct 21, 2015

samsonasik Oct 21, 2015

ghost commented Oct 22, 2015

samsonasik commented Oct 22, 2015

acelaya commented Oct 22, 2015

samsonasik commented Oct 22, 2015

samsonasik commented Oct 22, 2015

samsonasik Oct 26, 2015

samsonasik commented Oct 26, 2015

samsonasik commented Oct 27, 2015

first drafts of theme switcher module #158

first drafts of theme switcher module #158

Conversation

ghost commented Oct 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Oct 20, 2015

Choose a reason for hiding this comment

ghost commented Oct 20, 2015

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

ghost commented Oct 21, 2015

samsonasik commented Oct 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Oct 22, 2015

samsonasik commented Oct 22, 2015

acelaya commented Oct 22, 2015

samsonasik commented Oct 22, 2015

samsonasik commented Oct 22, 2015

Choose a reason for hiding this comment

samsonasik commented Oct 26, 2015

samsonasik commented Oct 27, 2015