Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

Reduced code duplication in FlashMessenger plugin #6073

Closed
wants to merge 2 commits into from
Closed

Reduced code duplication in FlashMessenger plugin #6073

wants to merge 2 commits into from

Conversation

pszczekutowicz
Copy link

  • Zend/Mvc/Controller/Plugin/FlashMessenger.php

- Zend/Mvc/Controller/Plugin/FlashMessenger.php
* @return FlashMessenger Provides a fluent interface
*/
public function addMessage($message)
public function addMessage($message, $namespace = null)
Copy link
Member

Choose a reason for hiding this comment

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

All these new $namespace parameters need related tests

Copy link
Author

Choose a reason for hiding this comment

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

Tests created

@Ocramius Ocramius changed the title Reduced code duplication Reduced code duplication in FlashMessenger plugin Apr 2, 2014
@Ocramius
Copy link
Member

Ocramius commented Apr 2, 2014

Marking as bc-break since you added parameters to the methods.

@Ocramius Ocramius added this to the 2.4.0 milestone Apr 3, 2014
@weierophinney
Copy link
Member

@Ocramius not necessarily a BC break, as the new parameters are optional; original behavior is preserved, and you can call them without the arguments.

@Ocramius
Copy link
Member

@weierophinney subclasses would still need change

@weierophinney
Copy link
Member

@Ocramius good point.

@pszczekutowicz You can make this BC safe by removing the default argument, and instead using func_num_args and func_get_arg in your methods. It's more code, but simplifies the story for those subclassing the plugin.

@Ocramius
Copy link
Member

@weierophinney the BC break is acceptable if we schedule this for 2.4 (imo)

@pszczekutowicz
Copy link
Author

Any conclusion @Ocramius @weierophinney? Should i commit the change?

@weierophinney
Copy link
Member

@pszczekutowicz I believe @Ocramius is saying he can merge it as-is to develop for release with 2.4.0.

@ThaDafinser
Copy link
Contributor

@pszczekutowicz thanks for the change
@Ocramius thank you for merging it not before 2.4.0 -> i exactly run into the BC break with subclasses

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants