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

groupStart() refactorization #2270

Merged
merged 1 commit into from
Sep 27, 2019

Conversation

michalsn
Copy link
Member

Description
Since we have now groupStart() and havingGroupStart() it's a good moment to refactor the code responsible for those methods a bit.

In this PR I want to introduce two new protected methods: groupStartPrepare() and groupEndPrepare(). These will take over the responsibility for handling all other group* methods.

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@jim-parry
Copy link
Contributor

It even looks like the user guide doesn't need updating ... groupStart() there doesn't show parameters :-/
Are you planning unit tests for this?
I don't see this PR as urgent as 2269, as there doesn't appear to be any BC here. Is that right?

@lonnieezell
Copy link
Member

I like this. As long as we have existing tests in place for the groups then this should be good and doesn't need any additional tests.

@michalsn
Copy link
Member Author

The parameters for groupStart() were marked in code as "internal use only" so we should be good - no BC changes.

We have tests for existing group methods and I think it should be enough.

@jim-parry jim-parry merged commit 04cd07e into codeigniter4:develop Sep 27, 2019
@michalsn michalsn deleted the feature/group_refactor branch September 29, 2019 05:35
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.

3 participants