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

Make extension hook methods protected - part 2 #11258

Closed
emteknetnz opened this issue May 22, 2024 · 9 comments
Closed

Make extension hook methods protected - part 2 #11258

emteknetnz opened this issue May 22, 2024 · 9 comments

Comments

@emteknetnz
Copy link
Member

emteknetnz commented May 22, 2024

Follow up to this comment

There's a list of of $doNotMakeHookProtected in the extension-protector script created for this issue

Methods that matches this name were left public because there are currently being called directly

Notes

  • The 6.0.0 changelog has already been updated to broadly mention that extension hook implementations have been changed from public to protected, so no need to update changelog

Acceptance criteria

  • Go through the list of these of $doNotMakeHookProtected methods and where possible and appropriate, update code so they are not being called directly
  • If it's not sensible to stop calling the method directly, look for where the extend() call is and evaluate whether this should be
    • Changed to call the method directly
    • Removed
    • Updated so the method in the call to extend() is not the same name as the one being called directly
  • Change visibility on remaining extension hook methods that aren't called directly from public to protected

CMSEditLink() PRs

Kitchen sink CI run
Recipe reporting job failing because we're forking framework

Other PRs

Kitchen sink CI run
Recipe reporting job failing because we're forking framework

Note these can be merged independently of the CMSEditLink PRs

@GuySartorelli GuySartorelli self-assigned this Aug 15, 2024
This was referenced Aug 15, 2024
@emteknetnz
Copy link
Member Author

There's a bit too much going on here to give me confidence, could we resolve and merge the (get)CMSEditLink() PRs first, rebase any of the non-(get)CMSEditLink() PRs and then redo the kitchen sink build

@GuySartorelli
Copy link
Member

GuySartorelli commented Aug 22, 2024

They're entirely separate sets of PRs which don't depend on one another in any way, but we can handle them that way if that's what you prefer.

@emteknetnz
Copy link
Member Author

@GuySartorelli Now that we're using getCMSEditLink() - could you please redo the kitchen-sink CI run with the updated PRs

@GuySartorelli
Copy link
Member

GuySartorelli commented Aug 22, 2024

Sure, but all we did was change the name of things so it'll be the exact same result. With maybe some need to rebase the PRs just to make the sink run go green even though that doesn't change the result of merging them 😅

@emteknetnz
Copy link
Member Author

Please update the sink run, it's unlikely though there may have been a small regression accidentally introduced. It should just a matter of:

  • running composer update -W --no-install on the branch of recipe-kitchen-sink
  • pushing the updated composer.lock file
  • updating the link in the issue

@GuySartorelli
Copy link
Member

GuySartorelli commented Aug 22, 2024

Please update the sink run

I'm going to, gimme some time lol

It should just a matter of

I have to rebase some of the PRs as well most likely, as I mentioned above. This isn't the card I'm currently working on, so you'll need to wait til I'm ready to do this.

@GuySartorelli
Copy link
Member

@emteknetnz You've approved #11338 but not merged it, but then you assigned the issue to me? Did you forget to hit merge?

@GuySartorelli
Copy link
Member

I've rebased the PRs that had cmseditlink changes and rerunning sink.
Likely I'll have to rebase others too, but I'm letting it run as-is before I do that.

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

No branches or pull requests

2 participants