Zf3 & ZfcUser 3 support by Danielss89 · Pull Request #27 · mtudor/ZfcUserImpersonate

Danielss89 · 2018-03-12T20:01:21Z

You should probably tag a new major release as this is breaking for people using ZfcUser 2 or 1

mtudor · 2018-03-22T18:21:22Z

Thanks for the PRs Daniel, just wanted to let you know I've seen them and will have a look over as soon as I can with a view to a merge :)

lenada · 2018-04-05T10:03:56Z

@mtudor is there anything I can do to get this PR merged/released? Would a code review from my side help?
I do not want to stress! I really appreciate your effort for creating and maintaining this module.
Also kudos to @Danielss89 for the zf3 updates.

mtudor · 2018-04-08T08:29:59Z

Hi @lenada - appreciate the offer and the intent "not to stress" :) I might take you up on it but give me a little time to do an initial skim over first just in case to save you any wasted work. Just getting my dev environment back up (had to reinstall) and then I'll get onto this (hopefully in the next 7 days). Please do feel free to drop me a prod if things go quiet on my end, though I will try to make sure they don't! ;)

lenada · 2018-04-18T13:38:27Z

@mtudor dropping you a prod 🤓
I did some further testing requiring this branch/PR as a package via composer (defining https://github.com/Danielss89/ZfcUserImpersonate.git under repositories). Except some minor autoloading issues this works fine within my zf3 test project. I will add some code comments regarding the autoloading.

lenada

@Danielss89 good work 👍, see minor comments below

lenada · 2018-04-18T13:45:01Z

-<?php
-// Generated by ZF2's ./bin/classmap_generator.php
-return array(
-  'ZfcUserImpersonate\Module'                                               => __DIR__ . '/Module.php',


I suggest moving the current Module.php to src/ZfcUserImpersonate/Module.php and adding a /Module.php containing

<?php require_once __DIR__ . '/src/ZfcUserImpersonate/Module.php';

That way we can omit the classmap in composer.json and the module becomes fully PSR-0/4 compliant.

Yep - I'm up for this. Do you think we ought to go PSR-4 and change the base directory from src/ZfcUserImpersonate to just src/. Is that more in-line with the standard these days?

If we move the files, a second PR makes more sense I would think.

lenada · 2018-04-18T13:53:09Z

 {
+    public function getConfig($env = null)
+    {
+        return include __DIR__ . '/config/module.config.php';


When applying the require_once approach, as described below, /../../ need to be added to the config file include.

Pending the decisions on PSR-0 / PSR-4.

mtudor · 2018-04-18T14:11:53Z

Just finishing off some critical day job stuff and then will aim to jump on this - thanks for the prod :)

mtudor

A few small tweaks and I think we're good to go. Thanks for your work on this @Danielss89, and for the review @lenada :)

mtudor · 2018-04-19T10:38:42Z

-<?php
-// Generated by ZF2's ./bin/classmap_generator.php
-return array(
-  'ZfcUserImpersonate\Module'                                               => __DIR__ . '/Module.php',


Yep - I'm up for this. Do you think we ought to go PSR-4 and change the base directory from src/ZfcUserImpersonate to just src/. Is that more in-line with the standard these days?

If we move the files, a second PR makes more sense I would think.

mtudor · 2018-04-19T10:40:20Z

 {
+    public function getConfig($env = null)
+    {
+        return include __DIR__ . '/config/module.config.php';


Pending the decisions on PSR-0 / PSR-4.

mtudor · 2018-04-19T10:41:07Z

 use ZfcUserImpersonate\Module\AbstractModule;

-class Module extends AbstractModule
+class Module implements ConfigProviderInterface


So, to clarify as much for me as anyone (but also as a sanity check), now that we don't have any closures in the service and controller config, we no longer require the getControllerConfig(...), getServiceConfig(...) and getViewHelperConfig(...) functions that the AbstractModule provides. Therefore you've essentially removed the AbstractModule functionality (it's based on our https://github.com/tccltd/TccAbstractModule, btw, and we tend to use it for getting up and running quickly). If I've understood the reasoning correctly, I agree that for a vendor module we should aim to be optimised and so the removal makes sense. If I understand correctly as well, we now have the benefit that all of the config is cached?

Finally, we're dropping autoloader related stuff because nowadays composer should really take care of all of that?

mtudor · 2018-04-19T15:18:26Z

+        'zfcuserimpersonate_adminController' => \ZfcUserImpersonate\Factory\Controller\AdminControllerFactory::class,
+        'zfcuser' => \ZfcUserImpersonate\Factory\Controller\UserControllerFactory::class,
    )
 );


This all looks great, including the two new factory classes. I've been wanting to do that for a while so really appreciate this change!

mtudor · 2018-04-19T15:19:15Z

+        'zfcuserimpersonate_module_options' => \ZfcUserImpersonate\Factory\Service\ModuleOptionsFactory::class,
+        'zfcuserimpersonate_user_service' => \ZfcUserImpersonate\Factory\Service\UserServiceFactory::class,
    ),
-    'view_helpers' => include('viewhelper.config.php'),


***** Make a decision on this new style of config separation *****

mtudor · 2018-04-22T12:39:40Z

+use Zend\ModuleManager\Feature\ConfigProviderInterface;
 use Zend\Mvc\MvcEvent;
 use Zend\View\Model\ViewModel;
 use ZfcUserImpersonate\Module\AbstractModule;


This can go now, right (AbstractModule has gone)?

mtudor · 2018-04-22T12:56:05Z

+                        ),
+                    ),
+                ),
+            ),


I wonder if we ought to leave this in module.config.routes.php and require it as you have done with controller, service and viewhelper configs?

mtudor · 2018-04-22T13:18:50Z

@@ -7,21 +7,14 @@
 */

 use Zend\Authentication\Storage\Session;


No need for this use anymore either right?

mtudor · 2018-04-22T13:20:53Z

@@ -11,15 +11,7 @@



Can get rid of the use statements above now these are handled in the factories?

mtudor · 2018-04-22T13:21:32Z

@@ -10,26 +10,7 @@



Can get rid of the use statement above now this is handled in the factory?

Danielss89 added 5 commits March 12, 2018 20:44

Initial ZF3 and ZfcUser 3 work

011acc0

ZF3 & ZfcUser 3 support

5af498b

ZF3 & ZfcUser 3 support

0631447

Use Factories

5eeecf4

Fix user controller factory

85c4f28

lenada mentioned this pull request Mar 27, 2018

zf3 version #25

Open

lenada reviewed Apr 18, 2018

View reviewed changes

mtudor reviewed Apr 22, 2018

View reviewed changes

Conversation

Danielss89 commented Mar 12, 2018

Uh oh!

mtudor commented Mar 22, 2018

Uh oh!

lenada commented Apr 5, 2018

Uh oh!

mtudor commented Apr 8, 2018

Uh oh!

lenada commented Apr 18, 2018

Uh oh!

lenada left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtudor Apr 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtudor commented Apr 18, 2018

Uh oh!

mtudor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtudor Apr 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lenada left a comment •

edited

Loading

mtudor Apr 19, 2018 •

edited

Loading

mtudor Apr 19, 2018 •

edited

Loading