Skip to content

Conversation

japaveh
Copy link

@japaveh japaveh commented Aug 1, 2016

In ZfcUser many factories have moved from module.php to module.config.php.

This resulted in the unwanted effect that the factories in ZfcUserDoctrineORM are ignored. In this PR the two overwritten factories are created as separate factories.

<?php
return array(
'doctrine' => array(
'service_manager' => array(
Copy link

@Kwido Kwido Aug 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use the FQCN?

    'aliases'   => array(
        'zfcuser_register_form_hydrator' => 'zfcuser_user_hydrator',
        'zfcuser_user_mapper'            => ZfcUserDoctrineORM\Mapper\User::class,
        'zfcuser_module_options'         => ZfcUserDoctrineORM\Options\ModuleOptions::class,
    ),
    'factories' => array(
        ZfcUserDoctrineORM\Mapper\User::class           => ZfcUserDoctrineORM\Factory\UserMapperFactory::class,
        ZfcUserDoctrineORM\Options\ModuleOptions::class => ZfcUserDoctrineORM\Factory\ModuleOptionsFactory::class,
    ),

Why? As it allows to easily refactor your code.

Why pass aliases into the factories array? If we want a Mapper\User, I rather get it by its FQCN instead of by a string as I've got to replace each string. Yet a good IDE will help you out anyway.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, but I wanted to keep the module as close as possible to the ZfcUser, but indeed, good practice for both modules would be to have the FQCN

@japaveh
Copy link
Author

japaveh commented Aug 1, 2016

I am a bit confused. In ZfcUser/master which is currently not really under develoment I see that the services are all registered in the module.config.php where the new 2.x branch has moved al the code to the Module.php. For me both is fine, but what is best practice

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants