search

Ocramius / PackageVersions

:package: Composer addon to efficiently get installed packages' version numbers
MIT License
3.22k stars 67 forks source link

Delete class file prior to writing it to ensure permissions #66

Closed peterrehm closed 6 years ago

peterrehm commented 6 years ago

Alternative for #47 with the same effect if you have a proper permission setup.

Eventually you can accept this as this does nothing other than deleting the file.

Ocramius commented 6 years ago

I'd need test additions for this to be included

peterrehm commented 6 years ago

I thought a while about this, and think this is the safest way and none of the current tests are failing. Why would you need another test? TBH I do not know what test you would require as it proves already that the expected behaviour is not changed.

As there are issues popping up here and then you might consider merging this.

Ocramius commented 6 years ago

Hmm, the point is verifying what is being fixed, and making sure it doesn't regress again: for that I need something that replicates the problem.

peterrehm commented 6 years ago

I see your point, but it is a known issue and obviously not breaking the tests. And also logically deleting prior to writing to the file should not be an issue.

I can't spend hours in trying to replicate the chown php issue for this in a test. Others have simply supressed the warning.

I think you have a good fix, you can consider if you want to merge it, otherwise feel free to close it.

Ocramius commented 6 years ago

I think you have a good fix, you can consider if you want to merge it, otherwise feel free to close it.

Same as previously (same issue): I won't close it, but there will be no merge without a test, sorry.

Yes, the scenario may as well be complex to reproduce, but without a reproducible test it will pop up again later on, and somebody will have to spend time debugging it again.

Until there is a test, this will stay in a limbo.

peterrehm commented 6 years ago

The delete prior to the rest is covered by tests. Therefore I do not understand that there is no interest in merging this as this eases the user experience in scenarios where only the php chown causes issues.

The difference in the previous PR I suppressed warnings, this time it is deleting the file and no error is suppressed. The intention by your chown is thus maintained.

I won't provide additional tests, again I thought this will be a good addition. I have a workaround in my projects.

Please consider, if this is your final opinion let's close this issue. I just wanted to help but I can't spend hours on this.

Ocramius commented 6 years ago

I understand your stance, but disagree with it, as I prefer to have certainty of any fixes.

The difference in the previous PR I suppressed warnings, this time it is deleting the file and no error is suppressed. The intention by your chown is thus maintained.

For future readers, this is the scenario I'd need to have in the test suite.

Closing here as you requested.

peterrehm commented 6 years ago

All you want a check is that the chown is right after the file is written?

To test it properly you would need to setup the permission which create the issue with chown, and then run the properly working script. This requires a file with group access rights but with a different owner.

Still this seems to much work for me whereas the functionality is covered by existing tests.

Ocramius commented 6 years ago

whereas the functionality is covered by existing tests.

The bug is not.

peterrehm commented 6 years ago

Yes, but as long as the tested functionality is covered you could easily accept that seeing that the potential effort of testing the bug is not worth it.

Bug again your library, your choice. As much as I am behind your general oppinion on tests, for me I weigh up if its really worth it and the potential side effects.

Ocramius commented 6 years ago

Existing functionality => existing tests Added code => new test

It is that simple.