Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Allow setting of custom user hydrator and user mapper from config file. #189

Closed
wants to merge 2 commits into from

6 participants

@mtudor

Using PR #174 as a base, a change to make overriding the user hydrator and user mapper easier. I have extended the work by ClemensSahs to use config data to determine the hydrator and mapper classes.

Note that this PR deals only with the custom user hydrator and user mapper parts of PR #174 and NOT the custom database field names.

@ClemensSahs

Pls add the options in the Readme, too.

@mtudor

@ClemensSahs Thanks for the review - just updated the README.md as requested.

@ClemensSahs ClemensSahs referenced this pull request from a commit in ClemensSahs/ZfcUser
@ClemensSahs ClemensSahs Revert "why we have a factory for the "user hydrator" but doesn't use…
… them?"

the abbility to set a custom hydrator and mapper will be done in #189
so the "8e6329e" will a conflict.

This reverts commit 8e6329e.
81d6070
@Saeven

+1 for this

@akrabat
Owner

I'm not sure of the benefit of this PR as it's easy enough to create a new zfcuser_user_mapper ServiceManager entry in the module that has your new mapper in it if you need to override.

Having said that, I'm open to arguments.

@mtudor

@akrabat Agreed - you can override the ServiceManager entry - and that's exactly what I was doing prior to suggesting this PR.

Specifying the User Mapper Class in config was inspired by the option (which already exists in ZfcUser) to specify the Entity Class in config. There is quite a lot of setup code in the zfcuser_user_mapper ServiceManager entry, which would have to be copied into the overriding ServiceManager entry just to change the User Mapper Class. It seemed somewhat cleaner to be able to set a config variable, as opposed to duplicating the surrounding ServiceManager code (which may well change in later versions of ZfcUser).

I suppose one question worth contemplating is "why should I be able to specify the User Entity class in config but not the User Mapper class?", which would be the status quo if this PR is not accepted.

Should the User Entity Class actually be a separate ServiceManager entry? zfcuser_user_entity? Instead of a config variable?

Currently I think I fall on the side of either this should all be done in config, as proposed, or the zfcuser_user_mapper ServiceManager entry needs breaking down into smaller entries so that less code needs to be overridden just to set a custom mapper.

I really am open to discussion, and interested in opinion, on this one as I can see the argument from both points of view.

@akrabat
Owner

Fair points. I can see your argument. As I'm tending towards SM overrides in my own code, I had forgotten that ZfcUser allows configuration of things that I would do via SM :)

If you could update this PR so that it can be merged, then I'm happy to merge it.

@mtudor

This PR has now been rebased from the latest master, and should be mergeable.

However, I have made a small additional change I'd appreciate feedback on first - this is contained in the second commit. Along the lines of allowing easier SM overriding, I have extracted the zfcuser_user_entity into its own SM entry, similar to the zfcuser_user_hydrator. This will allow implementers to override the user entity configuration without having to override the entire zfcuser_user_mapper SM entry.

Arguably, this could remove the need for the user entity config setting (at least for now, until the config of that object becomes more involved). Arguably, it doesn't remove the need for the user mapper config setting (as there is still a lot of code in that zfcuser_user_mapper SM entry that would probably need to be duplicated).

For consistency and flexibility, however, I would vote for sticking with the config options and separated SM entries. We now have the option of overriding using either the SM entry (for altering the way the objects are configured) or the config setting (to change the object classes themselves).

If you're happy with this how it is, @akrabat, then merge away. If you'd prefer me to revert the latter change, let me know. If you want a bit more discussion, I'll place my brain on stand-by ;-)

@EvanDotPro
Owner

I have nothing against this PR -- but I will say the original rationale behind having the entity class configurable and not the mapper class was simply because the entity was not pulled from the service manager, thus you couldn't easily just override a service entry to replace it.

That said, I don't think there's anything wrong with this PR in general -- it's merely a convenience factor to avoid duplicating the factory code. Alternatively we could have the mapper factory be its own class with the mapper class name as a property. Overridding would then involve extending the factory, re-defining the property, and then replacing the factory class name... Any sane person will vote for a config parameter, of course. I just wanted to show that there's another way to architect it to avoid duplicate factory code but still allow overriding without a configuration value. :)

Anyway, :+1: from me.

@mtudor

Thanks for the vote of support @EvanDotPro! And for taking the time to explain both the original rationale and an alternative option for overriding without duplicating factory code. I'd completely forgotten that you can use a Service Factory Class instead of a closure in the SM config. I'll probably end up using that little trick in my own code, actually! When the setup gets really involved, I can see how that would become even more useful.

Anyway, as everyone seems happy to include the convenience of the config parameter, I've gone ahead and rebased against the latest master so this should now merge cleanly.

Cheers all!

@mtudor

Rebased on top of latest master again.

@EvanDotPro Merge me, merge me, merge me ;-)

@mtudor

Rebased - not sure if it's bad form to keep bumping this to the top (apologies if so!). But I think it was ratified and I've just rebased it again so I don't want it getting lost down at the bottom of the issues list :)

mtudor added some commits
@mtudor mtudor Allow setting of custom user hydrator and user mapper from config file.
Allow setting of custom user hydrator and user mapper from config file.

Updated README.md to include the 'user_mapper_class' and 'user_hydrator_class' config options.
30f5c43
@mtudor mtudor Extracted the creation of the User Entity into a separate ServiceMana…
…ger entry, for easier overriding.
4bb96c3
@spiffyjr
Owner

@EvanDotPro @akrabat input? Mergable?

Closing per https://github.com/ZF-Commons/ZF-Commons/wiki/ZfcUser-1.0-to-2.0-PR-queue-handling but I give @akrabat and @EvanDotPro the :thumbsup: to merge. I'm not very familiar with the Zend\Db portion or I would merge this myself.

@spiffyjr spiffyjr closed this
@mtudor

If you guys want to merge this, and it would help if I rebase it for you, just give me the nod.

@mtudor mtudor referenced this pull request from a commit in BinaryKitten/ZfcUser
@BinaryKitten BinaryKitten make use of the zfcuser_user_hydrator system definition aadbdc6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 28, 2013
  1. @mtudor

    Allow setting of custom user hydrator and user mapper from config file.

    mtudor authored mtudor committed
    Allow setting of custom user hydrator and user mapper from config file.
    
    Updated README.md to include the 'user_mapper_class' and 'user_hydrator_class' config options.
  2. @mtudor

    Extracted the creation of the User Entity into a separate ServiceMana…

    mtudor authored mtudor committed
    …ger entry, for easier overriding.
This page is out of date. Refresh to see the latest.
View
17 Module.php
@@ -150,18 +150,23 @@ public function getServiceConfig()
return $form;
},
+ 'zfcuser_user_entity' => function ($sm) {
+ $entityClass = $sm->get('zfcuser_module_options')->getUserEntityClass();
+ return new $entityClass;
+ },
+
'zfcuser_user_hydrator' => function ($sm) {
- $hydrator = new \Zend\Stdlib\Hydrator\ClassMethods();
- return $hydrator;
+ $hydratorClass = $sm->get('zfcuser_module_options')->getUserHydratorClass();
+ return new $hydratorClass;
},
'zfcuser_user_mapper' => function ($sm) {
$options = $sm->get('zfcuser_module_options');
- $mapper = new Mapper\User();
+ $mapperClass = $options->getUserMapperClass();
+ $mapper = new $mapperClass;
$mapper->setDbAdapter($sm->get('zfcuser_zend_db_adapter'));
- $entityClass = $options->getUserEntityClass();
- $mapper->setEntityPrototype(new $entityClass);
- $mapper->setHydrator(new Mapper\UserHydrator());
+ $mapper->setEntityPrototype($sm->get('zfcuser_user_entity'));
+ $mapper->setHydrator($sm->get('zfcuser_user_hydrator'));
$mapper->setTableName($options->getTableName());
return $mapper;
},
View
6 README.md
@@ -169,6 +169,12 @@ The following options are available:
- **user_entity_class** - Name of Entity class to use. Useful for using your own
entity class instead of the default one provided. Default is
`ZfcUser\Entity\User`.
+- **user_mapper_class** - Name of Mapper class to use. Useful for using your own
+ mapper class instead of the default one provided. Default is
+ `ZfcUser\Mapper\User`.
+- **user_hydrator_class** - Name of Hydrator class to use. Useful for using your
+ own hydrator class instead of the default one provided. Default is
+ `ZfcUser\Mapper\UserHydrator`.
- **enable_username** - Boolean value, enables username field on the
registration form. Default is `false`.
- **auth_identity_fields** - Array value, specifies which fields a user can
View
16 config/zfcuser.global.php.dist
@@ -24,6 +24,22 @@ $settings = array(
//'user_entity_class' => 'ZfcUser\Entity\User',
/**
+ * User Model Mapper Class
+ *
+ * Name of Mapper class to use. Useful for using your own mapper class
+ * instead of the default one provided. Default is ZfcUser\Mapper\User.
+ */
+ //'user_mapper_class' => 'ZfcUser\Mapper\User',
+
+ /**
+ * User Model Hydrator Class
+ *
+ * Name of Hydrator class to use. Useful for using your own hydrator class
+ * instead of the default one provided. Default is ZfcUser\Mapper\UserHydrator.
+ */
+ //'user_hydrator_class' => 'ZfcUser\Mapper\UserHydrator',
+
+ /**
* Enable registration
*
* Allows users to register through the website.
View
54 src/ZfcUser/Options/ModuleOptions.php
@@ -76,6 +76,16 @@ class ModuleOptions extends AbstractOptions implements
/**
* @var string
*/
+ protected $userMapperClass = 'ZfcUser\Mapper\User';
+
+ /**
+ * @var string
+ */
+ protected $userHydratorClass = 'ZfcUser\Mapper\UserHydrator';
+
+ /**
+ * @var string
+ */
protected $userLoginWidgetViewTemplate = 'zfc-user/user/login.phtml';
/**
@@ -496,6 +506,50 @@ public function getUserEntityClass()
}
/**
+ * set user mapper class name
+ *
+ * @param string $userMapperClass
+ * @return ModuleOptions
+ */
+ public function setUserMapperClass($userMapperClass)
+ {
+ $this->userMapperClass = $userMapperClass;
+ return $this;
+ }
+
+ /**
+ * get user mapper class name
+ *
+ * @return string
+ */
+ public function getUserMapperClass()
+ {
+ return $this->userMapperClass;
+ }
+
+ /**
+ * set user hydrator class name
+ *
+ * @param string $userHydratorClass
+ * @return ModuleOptions
+ */
+ public function setUserHydratorClass($userHydratorClass)
+ {
+ $this->userHydratorClass = $userHydratorClass;
+ return $this;
+ }
+
+ /**
+ * get user hydrator class name
+ *
+ * @return string
+ */
+ public function getUserHydratorClass()
+ {
+ return $this->userHydratorClass;
+ }
+
+ /**
* set password cost
*
* @param int $passwordCost
Something went wrong with that request. Please try again.