New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: messageName() method is abstract by default #72

Closed
prolic opened this Issue Aug 10, 2018 · 12 comments

Comments

Projects
None yet
6 participants
@prolic
Member

prolic commented Aug 10, 2018

I would like to propose that the messageName() method is abstract by default (which means you have to implement it on every class). The MessageFactory hence needs a message map, in order to create an object from message data, because the messageName doesn't have to be the class name. Class name as messsage name by default is removed.

Reasons:

  • for prooph/event-store-client we need messageMap anyway
  • it allows custom message names by default, lots of users asked on how not to have class name as message name and needed to write custom message factories in order to do this. Even with blog posts about this out there, the question came again and again. Actually nobody I'm aware of wants to have the class name as message name, hence why not change it now?

Thoughts?

@enumag @fritz-gerneth @basz

@prolic prolic added the BC break label Aug 10, 2018

@basz

This comment has been minimized.

Show comment
Hide comment
@basz

basz Aug 10, 2018

Member

agreed!

Member

basz commented Aug 10, 2018

agreed!

@enumag

This comment has been minimized.

Show comment
Hide comment
@enumag

enumag Aug 10, 2018

Contributor

Yeah, agreed.

Contributor

enumag commented Aug 10, 2018

Yeah, agreed.

@prolic

This comment has been minimized.

Show comment
Hide comment
@prolic

prolic Aug 10, 2018

Member

someone wanna provide a PR?

Member

prolic commented Aug 10, 2018

someone wanna provide a PR?

@mbadolato

This comment has been minimized.

Show comment
Hide comment
@mbadolato

mbadolato Aug 10, 2018

I'm all for it!

mbadolato commented Aug 10, 2018

I'm all for it!

@fritz-gerneth

This comment has been minimized.

Show comment
Hide comment
@fritz-gerneth

fritz-gerneth Aug 10, 2018

All for it. Think we can remove the implementation, the presence of that method is already enforced by the Message interface anyway

fritz-gerneth commented Aug 10, 2018

All for it. Think we can remove the implementation, the presence of that method is already enforced by the Message interface anyway

@enumag

This comment has been minimized.

Show comment
Hide comment
@enumag

enumag Aug 10, 2018

Contributor

The problem is that in my opinion the method should be static which it is not. When you find all messages via some composer autoloader magic or something like symfony/finder and want to build the map you need to call the method. Sure you can call non-static method by creating an instance without constructor using reflection but it's not ideal. What do you guys think?

EDIT: If we do change messageName to static we should do the same with messageType as well.

Contributor

enumag commented Aug 10, 2018

The problem is that in my opinion the method should be static which it is not. When you find all messages via some composer autoloader magic or something like symfony/finder and want to build the map you need to call the method. Sure you can call non-static method by creating an instance without constructor using reflection but it's not ideal. What do you guys think?

EDIT: If we do change messageName to static we should do the same with messageType as well.

@fritz-gerneth

This comment has been minimized.

Show comment
Hide comment
@fritz-gerneth

fritz-gerneth Aug 10, 2018

@enumag We're usually exposing the message name as a constant anyway. The method only returns the constant's value:

final class OurAwesomeCommand extends Command implements PayloadConstructable
{
    use PayloadTrait;

    public const COMMAND_NAME = 'message-name-string';

    protected $messageName = self::COMMAND_NAME;
}

fritz-gerneth commented Aug 10, 2018

@enumag We're usually exposing the message name as a constant anyway. The method only returns the constant's value:

final class OurAwesomeCommand extends Command implements PayloadConstructable
{
    use PayloadTrait;

    public const COMMAND_NAME = 'message-name-string';

    protected $messageName = self::COMMAND_NAME;
}
@callistino

This comment has been minimized.

Show comment
Hide comment
@callistino

callistino Aug 10, 2018

I used all tree (const, method and property) I build a map too like @enumag mentions for a custom NameMessageFactory and so far the constant helps but it's a construct that we shouldn't need to use if the method was static.

callistino commented Aug 10, 2018

I used all tree (const, method and property) I build a map too like @enumag mentions for a custom NameMessageFactory and so far the constant helps but it's a construct that we shouldn't need to use if the method was static.

@fritz-gerneth

This comment has been minimized.

Show comment
Hide comment
@fritz-gerneth

fritz-gerneth Aug 10, 2018

@callistino we sure are uing it for the same reason. But this map does not exist for the reason ob getting the message name when you have the message. For this the existing method is used exactly.
It is used for the reverse way of getting the class name for a respective message name. For this no static method can help you anyway.

Besides this, I personally think having static methods on interfaces is a bad design. The message name is a property of a given message object. A static one, but yet a property of the object. Having some gettings of a class static just because they can makes a class much less of a class.

fritz-gerneth commented Aug 10, 2018

@callistino we sure are uing it for the same reason. But this map does not exist for the reason ob getting the message name when you have the message. For this the existing method is used exactly.
It is used for the reverse way of getting the class name for a respective message name. For this no static method can help you anyway.

Besides this, I personally think having static methods on interfaces is a bad design. The message name is a property of a given message object. A static one, but yet a property of the object. Having some gettings of a class static just because they can makes a class much less of a class.

@prolic

This comment has been minimized.

Show comment
Hide comment
@prolic

prolic Aug 11, 2018

Member

Implemented on develop-branch: https://github.com/prooph/common/tree/develop

Member

prolic commented Aug 11, 2018

Implemented on develop-branch: https://github.com/prooph/common/tree/develop

@callistino

This comment has been minimized.

Show comment
Hide comment
@callistino

callistino Aug 12, 2018

@fritz-gerneth We have a symphony container compiler pass that automatically generates the NameMessageFactory class and it uses reflection to get the name for any class implementing HasMessageName interface to build the map. With a static method we wouldn't need an instance, or a container reflector class. That's why we use the constant but a static method would make it unnecessary.

callistino commented Aug 12, 2018

@fritz-gerneth We have a symphony container compiler pass that automatically generates the NameMessageFactory class and it uses reflection to get the name for any class implementing HasMessageName interface to build the map. With a static method we wouldn't need an instance, or a container reflector class. That's why we use the constant but a static method would make it unnecessary.

@prolic

This comment has been minimized.

Show comment
Hide comment
@prolic

prolic Aug 20, 2018

Member

This library will receive support until December 31, 2019 and will then be deprecated.

For further information see the official announcement here: https://www.sasaprolic.com/2018/08/the-future-of-prooph-components.html

Member

prolic commented Aug 20, 2018

This library will receive support until December 31, 2019 and will then be deprecated.

For further information see the official announcement here: https://www.sasaprolic.com/2018/08/the-future-of-prooph-components.html

@prolic prolic closed this Aug 20, 2018

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