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

use hasMethod instead of methodExists #127

Merged
merged 3 commits into from
Jul 20, 2023

Conversation

wernerkrauss
Copy link

@wernerkrauss wernerkrauss commented Jul 11, 2023

@wernerkrauss
Copy link
Author

I worked against the 2 branch, as the current project is on SS4.

@GuySartorelli
Copy link
Member

Thanks for this.
In future, please make sure to include (at a bare minimum) the issue that the PR relates to in the PR description. I've added it for you this time.

@GuySartorelli
Copy link
Member

GuySartorelli commented Jul 16, 2023

You say you worked against the 2 branch - but this PR targets the 3 branch. I think this is an enhancement, and therefore it should be targetted against the 3 branch, but just in case I'll give you a chance to respond before I merge this.

If this is fixing a bug - please describe exactly what the bug is, and retarget this PR to the 2.3 branch.

@wernerkrauss
Copy link
Author

IMHO it's kind of a bug, cause normally one can decorate classes with extensions and the methods in the extensions work magically like normal methods. That's the whole reason why hasMethod()exists.

This fix enables you to modify a 3rd party lumberjack config the "Silverstripe way" in an Extension subclass without touching 3rd party code.

@wernerkrauss wernerkrauss changed the base branch from 3 to 2.3 July 20, 2023 13:06
@GuySartorelli
Copy link
Member

Hmmmm I'm a fence sitter on the definition of bug vs enhancement in this case but it's a very low risk change that doesn't affect the API at all so I'm okay with it being released as a patch.

@GuySartorelli GuySartorelli merged commit fcdfcdf into silverstripe:2.3 Jul 20, 2023
9 checks passed
@wernerkrauss
Copy link
Author

Thanks for helping me getting the PR right and for merging it.

@GuySartorelli
Copy link
Member

No worries, thanks for your efforts submitting this 👍

GuySartorelli added a commit to creative-commoners/silverstripe-lumberjack that referenced this pull request Jul 30, 2023
Revert "FIX Allow extension methods to be used in lumberjack  (silverstripe#127)"

This reverts commit fcdfcdf.
emteknetnz pushed a commit that referenced this pull request Jul 31, 2023
Revert "FIX Allow extension methods to be used in lumberjack  (#127)"

This reverts commit fcdfcdf.
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.

None yet

3 participants