Skip to content

Commit

Permalink
Make message class inheritable again
Browse files Browse the repository at this point in the history 
Some key properties of the class Message are private preventing any proper inheritance. Open them up by making them protected
  • Loading branch information
@mtangoo
mtangoo authored Aug 21, 2023
1 parent 70c4d71 commit c656520
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions src/Message.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@
*/
class Message extends BaseMessage implements MessageWrapperInterface
{
private Email $email;
private string $charset = 'utf-8';
protected Email $email;
protected string $charset = 'utf-8';
public function __construct(array $config = [])
{
$this->email = new Email();
Expand Down

4 comments on commit c656520

@SamMousa
Copy link
Contributor

Choose a reason for hiding this comment

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

@mtangoo this is a design error. These properties should not be opened up for "proper inheritance".

  • First of all this type of inheritance on 3rd party code should be avoided; now this is part of our public API since people might depend on it
  • In this specific case you can use the setters and getters which are public so there's no need to make the property protected.

@mtangoo
Copy link
Contributor Author

Choose a reason for hiding this comment

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

True that.
Feel free to add Getters/Setters and revert the fields to private.
To that end, the class should be final too.

Since the upcoming version will probably have some BC, it shouldn't be a big deal. Just add in changelog for people to use getters/setters as the fields are going private!

Thanks for your efforts and time

@SamMousa
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer final regardless; but do / did you have a use case for extending it?

@mtangoo
Copy link
Contributor Author

Choose a reason for hiding this comment

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

do / did you have a use case for extending it?

I do not even remember what use case resulted in this. It must be one of the issues. But definitely it was not good decision. Setters/Getters and final should have been the way to go!

Please sign in to comment.