Skip to content
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

run attributeValuesToBeLogged method before getDescripotionForEvent in LogsActivity trait #730

Merged
merged 5 commits into from
Sep 16, 2020

Conversation

ahmedabdel3al
Copy link

in this PR i thank we can run attributeValuesToBeLogged method in trait LogsActivity
and pass result as second parameter in getDescriptionForEvent which it useful to get Appropriate description for attributes changed

$description = $model->getDescriptionForEvent($eventName, $attrs);

…Event and also pass attributeValuesToBeLogged to getDescriptionForEvent
@ahmedabdel3al ahmedabdel3al changed the title run attributeValuesToBeLogged method before getDescripotionForevent in LogsActivity run attributeValuesToBeLogged method before getDescripotionForEvent in LogsActivity trait May 15, 2020
@Gummibeer
Copy link
Collaborator

Hey,
thanks for your work!

Even if all tests pass this is a BC for me - because the signature of getDescriptionForEvent() is changed and this method is meant to get overridden in most cases. So at first the signature should be adjusted to public function getDescriptionForEvent(string $eventName, array $attributes): string and this will throw a PHP Warning Declaration of XYZ should be compatible with XYZ in XYZ

But I would like to ask @freekmurze what he thinks about this change in general and how he would rate it's impact and release.

@ahmedabdel3al
Copy link
Author

@Gummibeer
thanks for your reply
i fixed signature of getDescriptionForEvent() to be public function getDescriptionForEvent(string $eventName, array $attributes): stringin LogsActivity trait
also in LogsActivityTest class in it_will_not_fail_if_asked_to_replace_from_empty_attribute method which contain getDescriptionForEventmethod
thank you ,

@Gummibeer Gummibeer merged commit 01392f7 into spatie:v4 Sep 16, 2020
@Gummibeer Gummibeer mentioned this pull request Sep 16, 2020
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants