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

Added save-method to RelationFactory. Closes #2832 #2833

Merged
merged 4 commits into from
Sep 21, 2021

Conversation

andersbjorkland
Copy link

Added save-method to RelationFactory, and tests to check if the save method persists a single Relation as well as a collection of Relation.

Copy link
Member

@bobdenotter bobdenotter left a comment

Choose a reason for hiding this comment

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

You included tests!! 💛

@bobdenotter
Copy link
Member

Could you run ECS and PHPStan on your branch?

vendor/bin/ecs check tests/php --fix
vendor/bin/phpstan --memory-limit=1G analyse -c phpstan.neon src

@andersbjorkland
Copy link
Author

Ah, I see I didn't run ECS. I did run phpstan though.

@andersbjorkland
Copy link
Author

Could you run ECS and PHPStan on your branch?

vendor/bin/ecs check tests/php --fix
vendor/bin/phpstan --memory-limit=1G analyse -c phpstan.neon src

Yeah, I'll do that. I'm sitting on a train, so just gotta dig up the charger.

@andersbjorkland
Copy link
Author

What is up with Ignored error pattern #Unreachable statement - code above always terminates# ?
image

@bobdenotter
Copy link
Member

What is up with Ignored error pattern #Unreachable statement - code above always terminates# ?

Sometimes we have a PHPstan, that is either a false positive, or we think it's fine. So we add it to this file to ignore: https://github.com/bolt/core/blob/master/phpstan.neon#L14

However, if an ignore is never triggered, PHPstan also flags it as a potential issue. So, i think in this case, the bug that resulted in the false positive might be fixed, you got a new PHPStan tag pulled in, and it flags it.

If that's the only failing test, we'll merge in your PR, and fix PHPStan in a follow up PR.

@andersbjorkland
Copy link
Author

PHPUnit 7.2 didn't like my tests. I can write my arrow functions as regular anonymous functions instead 🤔

@bobdenotter
Copy link
Member

Yes, let's..

With Bolt, we're sticking with the same minimum requirements as Symfony, which means that we should support PHP 7.2.9, for the time being.

@andersbjorkland
Copy link
Author

Had to debug my unit test. I filtered an array and reset the index of the array - is what I thought. I actually just reset the pointer. So I switched out the reset()-function for the array_values()-function in the method getNonRelatedEntryIds.

@bobdenotter
Copy link
Member

Thanks again, @andersbjorkland

@bobdenotter bobdenotter merged commit 7a11638 into bolt:master Sep 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants