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

Update HasMeta entity to correct errors #440

Merged
merged 2 commits into from
Mar 20, 2024

Conversation

dgvirtual
Copy link
Contributor

@dgvirtual dgvirtual commented Mar 17, 2024

preventing it working with custom classes

Here I implement a different approach that was suggested in this comment in previous pull request #428.

@lonnieezell, would that work? I see deptrac and phpstan errors, but they are not related to this PR afaik.

@datamweb
Copy link
Contributor

would that work? I see deptrac and phpstan errors, but they are not related to this PR afaik.

I sent PR #441 .

@dgvirtual
Copy link
Contributor Author

@datamweb, once your PR is accepted, how do I rerun the tests on my PRs? I know I could do that by adding more commits to PR, but I do not want to...

@datamweb
Copy link
Contributor

once your PR is accepted, how do I rerun the tests on my PRs? I know I could do that by adding more commits to PR, but I do not want to...

You need to rebase . After merged my PR, you can proceed as follows:

git fetch upstream
git switch develop
git merge upstream/develop
git branch syncMeta-fix.bk syncMeta-fix
git switch syncMeta-fix
git rebase upstream/develop
git push --force-with-lease origin syncMeta-fix

Copy link
Owner

@lonnieezell lonnieezell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this approach is the simplest. Just a couple of small tweaks, please.

src/Core/Traits/HasMeta.php Outdated Show resolved Hide resolved
docs/users_and_security/user_meta.md Outdated Show resolved Hide resolved
src/Core/Traits/HasMeta.php Show resolved Hide resolved
src/Users/Libraries/UserCells.php Outdated Show resolved Hide resolved
src/Users/User.php Outdated Show resolved Hide resolved
preventing it working with custom classes
Make syncMeta() deal gracefully with situations when meta array is not set
@dgvirtual
Copy link
Contributor Author

hi @lonnieezell, I applied most changes you suggested, but not through the Github interface; not sure how to proceed from this point (and there are more phpstan errors not related to my changes...)

@lonnieezell
Copy link
Owner

@dgvirtual If it's in the same branch as this PR was originally created in you can just push the changes up.

Ignore PHPStan. I'll have changes to that in the near future :D

@dgvirtual
Copy link
Contributor Author

@dgvirtual If it's in the same branch as this PR was originally created in you can just push the changes up.

I did just that - see the second commit. But after going through all buttons to address requested changes I still see "Merging is blocked". (Before pushing the second commit I also did a rebase on current develop branch - maybe that was a mistake?).

@lonnieezell
Copy link
Owner

Oh - merging was blocked because I had requested changes and hadn't done another review approving it. Just did that. Will merge now. Thanks!

@lonnieezell lonnieezell merged commit 0bd06e1 into lonnieezell:develop Mar 20, 2024
6 of 10 checks passed
@dgvirtual
Copy link
Contributor Author

Oh - merging was blocked because I had requested changes and hadn't done another review approving it. Just did that. Will merge now. Thanks!

Great, thank you!

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