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

Fix uninitialized Base::unique #309

Closed
wants to merge 2 commits into from
Closed

Conversation

simPod
Copy link

@simPod simPod commented Apr 11, 2021

What is the reason for this PR?

  • A new feature
  • Fixed an issue

Author's checklist

Summary of changes

Base::unique property was not initialized in constructor and did not allow null. That may cause issues in classes accessing it while extending Base

Review checklist

  • All checks have passed
  • Changes are approved by maintainer

@pimjansen
Copy link

@GrahamCampbell guess we can accept this BC right after updating the definition?

src/Faker/Provider/Base.php Outdated Show resolved Hide resolved
@pimjansen
Copy link

Can we ensure that the BC check passes?

@simPod
Copy link
Author

simPod commented Apr 13, 2021

@pimjansen how? The null value was always there but it was typed as non-null so I think this is BC.

@pimjansen
Copy link

@pimjansen how? The null value was always there but it was typed as non-null so I think this is BC.

Ensure that the BC check allows it for now

@bram-pkg
Copy link
Member

This looks fine to me.

@bram-pkg bram-pkg added the documentation Improvements or additions to documentation label Apr 15, 2021
@pimjansen
Copy link

@simPod will you update the PR accordingly the code guidelines? We need a green build to pass

@simPod
Copy link
Author

simPod commented Apr 25, 2021

@pimjansen how? The BC change is the point of this PR. $unique could always be null so it was a bug not to have it in type definition. Now the bug is fixed and it violates BC check, yes 🤔

@pimjansen
Copy link

@pimjansen how? The BC change is the point of this PR. $unique could always be null so it was a bug not to have it in type definition. Now the bug is fixed and it violates BC check, yes

In that case it will be a no fix since we are not running BC changes in our update policy for the current stream of Faker

@pimjansen pimjansen closed this Apr 25, 2021
@simPod
Copy link
Author

simPod commented Apr 25, 2021

Should I remove "Fix" from a title and target a version where this can be fixed?

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

Successfully merging this pull request may close these issues.

None yet

4 participants