Skip to content

Allow individual isDraft in props to work #5043

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.

Sign up for GitHub

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

Jump to bottom
Merged
distantnative merged 5 commits into from
Feb 25, 2023
Merged

Allow individual isDraft in props to work #5043

distantnative merged 5 commits into from
Feb 25, 2023

Conversation

@SeriousKen
Copy link
Contributor

@SeriousKen SeriousKen commented Feb 5, 2023 •
edited by distantnative
Loading

Fix to allow individual isDraft to override the factory default if set in the array of props (issue #5041)

This PR …

Fixes

Issue #5041

Breaking changes

None

Ready?

  • Unit tests for fixed bug/feature
  • In-code documentation (wherever needed)
  • Tests and checks all pass

For review team

@distantnative
Copy link
Member

distantnative commented Feb 5, 2023

@SeriousKen Thanks for the PR!
Though I think this fix is not fully correct. I think right now the issue is that if the $draft parameter is set to false as default it still overwrites any custom isDraft - that's a bug indeed.

However, when somebody is explicitly calling the method with a $draft parameter, I think this should overwrite it, which your PR would prevent.

So I think we should change the parameter default to null and do nothing when it's null but ensure the overwrite when an actual value is passed.

@SeriousKen
Copy link
Contributor Author

SeriousKen commented Feb 5, 2023

I've updated the PR to reflect the changes in priority of the $draft parameter vs individual isDraft prop.

@distantnative
Copy link
Member

distantnative commented Feb 12, 2023

@SeriousKen same here, if you could add unit tests that would be splendid. if that is out of your comfort zone, let us know :)

@distantnative distantnative added this to the 3.9.2 milestone Feb 12, 2023
@SeriousKen
Copy link
Contributor Author

SeriousKen commented Feb 12, 2023

I've added a test case that should cover this PR.

SeriousKen and others added 4 commits February 17, 2023 00:03
@SeriousKen @afbora
Allow individual isDraft in props to work
280a359 
Fix to allow individual isDraft to override the factory default if set in the array of props (issue #5041)
@SeriousKen @afbora
Update Pages.php
9f0d673 
Change default for `$draft` to be null. If `$draft` is explicitly set then this overrides the individual `isDraft` prop, if neither are set it defaults to `false` (the original default value for `$draft`.
@SeriousKen @afbora
Added unit test
023a336
@afbora
Fix code style
6c52fd3
afbora
afbora reviewed Feb 16, 2023
View reviewed changes
@afbora afbora linked an issue Feb 16, 2023 that may be closed by this pull request
Pages::factory() ignores individual isDraft statuses #5041
Closed
afbora
afbora previously approved these changes Feb 16, 2023
View reviewed changes
@distantnative @afbora
Update src/Cms/Pages.php
24fb878 
Co-authored-by: Ahmet Bora <[email protected]>
@distantnative distantnative merged commit 91cbb93 into getkirby:develop Feb 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lukasbestle lukasbestle lukasbestle left review comments

@afbora afbora afbora left review comments

@distantnative distantnative distantnative approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

3.9.2

Development

Successfully merging this pull request may close these issues.

Pages::factory() ignores individual isDraft statuses

4 participants

@SeriousKen @distantnative @lukasbestle @afbora