Skip to content

Conversation

xiaohutai
Copy link
Owner

Requires opt-in via enabledisplaynames in config

I'm not 100% about adding a whole service for one feature.
Ideally this key, value is added before parse?

Fixes #84

/** @var Users $users */
protected $users;

public function __construct(Config $config, ResourceManager $resourceManager, MetadataDriver $metadata, Users $users)
Copy link
Contributor

Choose a reason for hiding this comment

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

If BC is being observed, this is a break.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Oh! Good point!

Thanks for checking @GawainLynch 👍

@xiaohutai
Copy link
Owner Author

Moved mandatory constructor param to public setUsers.

NB: Good resource on BC: https://symfony.com/doc/current/contributing/code/bc.html

@xiaohutai xiaohutai force-pushed the feature/add-displaynames branch from d3f079b to 0de2c89 Compare July 30, 2018 10:21
@xiaohutai xiaohutai force-pushed the feature/add-displaynames branch from 0de2c89 to ec57a3b Compare July 30, 2018 11:01
@xiaohutai xiaohutai changed the title Add display names feature [WIP] Add display names feature Jul 30, 2018
@HorlogeSkynet
Copy link

Hey @xiaohutai ! Are we all good here ? Is this merge-able and bump-able to v4.0 ? 😃
Thanks, see you 👋

@xiaohutai xiaohutai merged commit 0d4344b into master Aug 2, 2018
@xiaohutai xiaohutai deleted the feature/add-displaynames branch August 2, 2018 10:09
@xiaohutai
Copy link
Owner Author

@HorlogeSkynet Watch out for v3.3.0 soon ™️

@HorlogeSkynet
Copy link

Thanks for all, see you 👋🏻

@HorlogeSkynet
Copy link

Wow, such rude boys we are... Just wanted to tell you this DID PERFECTLY THE JOB. Thanks again. Bye 👋

@xiaohutai
Copy link
Owner Author

😍
I appreciate your message. Glad you like it!

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.

3 participants