Skip to content
New issue

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

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

Already on GitHub? # to your account

Allowing Methods to be set or unset in ModelHooks #1198

Merged
merged 6 commits into from
Apr 3, 2021
Merged

Allowing Methods to be set or unset in ModelHooks #1198

merged 6 commits into from
Apr 3, 2021

Conversation

jenga201
Copy link

@jenga201 jenga201 commented Apr 1, 2021

Summary

Expanding PR #945 to include support in ModelHooks to unset and set Methods.
This can be useful for defining class inheritance properly for the IDE.

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist

  • Existing tests have been adapted and/or new tests have been added
  • Add a CHANGELOG.md entry
  • Update the README.md
  • Code style has been fixed via composer fix-style

Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

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

Can of worms has been opened!

Can you add some tests maybe? 🤔

@jenga201
Copy link
Author

jenga201 commented Apr 2, 2021

Added in some tests

Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@barryvdh 👍

@mfn mfn requested a review from barryvdh April 2, 2021 22:17
@@ -749,7 +749,7 @@ public function setProperty($name, $type = null, $read = null, $write = null, $c
}
}

protected function setMethod($name, $type = '', $arguments = [], $comment = '')
public function setMethod($name, $type = '', $arguments = [], $comment = '')
Copy link
Collaborator

Choose a reason for hiding this comment

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

As with #945 (comment) , this will bring the same kind of breaking change.

So maybe in this case we should bump the major?

Copy link
Owner

Choose a reason for hiding this comment

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

I don't know, I think the impact is pretty small. The intended use case is not to provide an extensible package for all methods imho.

Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

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

Let's merge it, but wait for a release a bit, see #1198 (comment)

@mfn mfn merged commit c1ddd30 into barryvdh:master Apr 3, 2021
@mfn mfn mentioned this pull request Apr 6, 2021
9 tasks
@mfn
Copy link
Collaborator

mfn commented Apr 9, 2021

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants