Skip to content

Conversation

maks-rafalko
Copy link

No description provided.

@jdreesen
Copy link

Be careful, the record() method is protected in the trait and public the class.
I think that's the reason one is named PrivateMessageRecorderCapabilities and the other one PublicMessageRecorder.

@jdreesen
Copy link

I don't think simply changing the method's visibility to public in the trait is the way to go here.

@jdreesen
Copy link

I'm not using traits much... but maybe leaving the method protected in the trait and using it with

use PrivateMessageRecorderCapabilities { record as public; }

would work?

@maks-rafalko
Copy link
Author

maks-rafalko commented Jan 30, 2017

I agree with your notices, but let's have a look at the documentation: http://simplebus.github.io/MessageBus/doc/message_recorder.html

class YourEntity implements RecordsMessages
{
...
}

The documentation says that Entity implements the interface RecordsMessages which has (obviously) a public method records, which is completely compatible with what I have changed

@maks-rafalko
Copy link
Author

@jdreesen yeah, looks like a good solution. Let's wait the travis build

@jdreesen
Copy link

Oh, you are right, but I think the docs are wrong here because the section describes privately recording messages (which is generally what you want to do in a domain entity - it should be able to record it's own messages, but no one else should be allowed to do this).

@coveralls
Copy link

coveralls commented Jan 30, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling d0bc478 on borNfreee:patch-1 into 2a7a6b7 on SimpleBus:master.

@maks-rafalko
Copy link
Author

maks-rafalko commented Jan 30, 2017

@jdreesen ok, I applied your suggestion and now the Entity will still have protected function while public recorder will reuse the trait with public method. What do you think?

//cc @matthiasnoback

@jdreesen
Copy link

I'm no maintainer of this project, and no great fan of traits, but as the trait exists anyways and differs with the class only in the visibility of one method I think this seems like a valid approach.
But the docs should be corrected, too.

@matthiasnoback
Copy link

Hi everyone, I'm sure @cmodijk was going to reply but I saw that passing by and thought it was an interesting suggestion - I didn't know PHP had the option to change visibility of trait methods when used, so... this would be a win-win situation I think. You don't always have to be DRY about everything, but in this case the implementation is not only accidentally the same, it is supposed to be the same. Thanks!

@cmodijk cmodijk merged commit dc9e467 into SimpleBus:master Feb 2, 2017
@cmodijk
Copy link
Member

cmodijk commented Feb 2, 2017

@borNfreee @jdreesen @matthiasnoback Tnx for the discussion i never new this was possible with traits. I just merged this in the code bases tnx!

@maks-rafalko maks-rafalko deleted the patch-1 branch September 27, 2017 13:52
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.

5 participants