customizable user mapper and hydrator #174

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

ClemensSahs commented Nov 6, 2012

This PR allows the ability, to overwrite the mapper and hydrator without a new implementation.
It allows a customizable with few lines as possible.

This PR was spitted in tow clearly parts:

  • for customisable hydrator and mapper class (PR #189)
  • for customisable database field names (PR #174)

ClemensSahs added some commits Nov 4, 2012

change single field mapping with mapMethodes
- add mapFieldForExtract
- add mapFieldForHydrate
- add reference operator to mapField attribute array ( performance )
- remove return array because we dont need this anymore
why we have a factory for the "user hydrator" but doesn't use them?
- change directly object build to factory
- change factory to the current UserHydrator

mtudor commented Nov 28, 2012

I actually just came here to propose something similar to the changes you've made to Module.php, so it's great to see that I'm not the only one who spotted the inability to customise the hydrator without overriding "zfcuser_user_mapper".

I wonder if we ought to take it one further and actually provide a config variable for the mapper and hydrator, such as there is for the entity class?

I'd be tempted to split this into two pull requests, too. One for the changes to make the hydrator and mapper customisable and another for the customisable table fields (should we also make the table name itself customisable?).

I think that would probably make it easier for Evan to accept the PR, rather than requiring him to wait until everything is perfect across both functionality changes?

mtudor commented Nov 28, 2012

@ClemensSahs I've extended your PR into PR #189. See what you think. I don't want to usurp your PR but it seemed the easiest way to offer an actionable suggestion.

PR #189 doesn't deal with the custom field / table names - I suggest we leave these in this PR to be considered separately.

Contributor

ClemensSahs commented Nov 29, 2012

@mtudor
hy cool, I knew that I'm not the only one.

You want split this, in tow PRs... hm... I don't know this make it easy to pull.
So i must clean up the changes in "\ZfcUser\Module", to solve all conflicts, right?

mtudor commented Nov 29, 2012

@ClemensSahs My logic behind the splitting of this PR is that you have implemented two distinct changes:

  1. The customisable hydrator and mapper;
  2. The customisable database field names.

If, when it comes to reviewing these changes, Rob or Evan (or whoever from the ZF-Commons team) is happy with one set of changes, but not the other, revisions will hold up both changes being accepted into the main code. If the PR is split, then one can be accepted without having to accept the other at the same time. I hope that makes sense!

If you are happy that PR #189 that I raised meets your requirements for [1. The customisable hydrator and mapper] then I suggest we make this current PR (#174) cover only [2. The customisable database field names].

So essentially, yes, if you revert your changes in Module.php then we should be good to go!

Contributor

ClemensSahs commented Nov 29, 2012

Yes this make sense.

Currently i have my overwrite this over factory in a custom module, so I am happy. ;)
But this is not pretty, so i hope both features will be merged.

What are you think about, this three way's

->where(array($this->fieldNames['id'] => $id));
->where(array(static::$FIELD_NAME_ID => $id));
->where(array($this->getFieldName('id') => $id));
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.
Contributor

ClemensSahs commented Nov 29, 2012

update the pull request description!

and revert the ZfcUser\Module conflict with PR #189

Contributor

akrabat commented Feb 2, 2013

What does this PR actually do now?

mtudor commented Feb 4, 2013

@akrabat It was re-factored to deal only with the ability to specify custom field names in the user table. It's not functionality that I personally need, right now, so I will leave @ClemensSahs to go into further detail.

However, I can see the potential usefulness of allowing custom table and fieldnames to be used, although I think my preference would be for these to be set in config rather than in the User Mapper. Of course we might just get into the debate of whether this would just be a Mapper override, and then we're considering whether overriding the mapper just to change table / fieldnames is overkill... ;-)

I haven't looked in much more detail at this stage into how this PR operates.

Contributor

ClemensSahs commented Feb 4, 2013

@akrabat
This PR currently give the ability to customize the mapping for Entry\User to database with overwrite the Mapper\User.

like:

class MyUserMapper extends \ZfcUser\Mapper\User {
    protected $tableName  = 'user_table';
    protected $fieldNames = array(
        'id'=>'id',
        'email'=>'email',
        'username'=>'username'
    );
}

A alternative was to change the fieldNames in a Object of \ZfcUser\Mapper\User, this is currently not implemented.

like:

$mapper = new \ZfcUser\Mapper\User();
$mapper->setFieldNames(array(...));

mtudor commented Feb 4, 2013

Just to draw attention to this commit aa3107b which seems to implement the tablename config option. Do we think that the option for fieldnames should follow this structure?

Contributor

ClemensSahs commented Feb 4, 2013

@mtudor
yes this is the alternative way I mean ;)

The question is this a better solution as overwriting? So I will commit it on this afternoon.

mtudor commented Feb 4, 2013

I think that if you follow the structure committed in aa3107b then you get the best of both worlds.

  • Change the fieldnames in config by altering the 'field_names' config property array.
  • Change the fieldnames by extending ZfcUser\Mapper\User and overwriting the $fieldNames property [you would also need to override the SM entry to avoid the names being overwritten by $mapper->setFieldNames($options->getFieldNames())].
  • Change the fieldnames at runtime by calling setFieldNames(...) / getFieldNames(...).

Though I feel that the majority of people would opt for the first of those options, you have flexibility to use whichever suits the situation.

I would wait for @akrabat to weigh in again, as he's an official ZfcUser maintainer, where I am not. I would also consider rebasing your PR off the master so that you are working with the most up to date accepted code in ZfcUser - that will help if it is agreed that matching the structure in the commit referenced above is the best course of action.

Not a bad idea but I would suggest moving the options to an Options class that extends Zend\Stdlib\AbstractOptions and having them lazy-loaded (via getOptions()).

https://github.com/ZF-Commons/ZF-Commons/wiki/ZfcUser-1.0-to-2.0-PR-queue-handling reopen when ready! Thanks!

Also, added to TODO doc for ZfcUser 2.0.

https://github.com/ZF-Commons/ZF-Commons/wiki/ZfcUser-2.0:-Additional-features

@spiffyjr spiffyjr closed this Jun 21, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment