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

[5.7] Revert breaking filesystem changes #26231

Merged
merged 1 commit into from
Oct 24, 2018
Merged

[5.7] Revert breaking filesystem changes #26231

merged 1 commit into from
Oct 24, 2018

Conversation

laurencei
Copy link
Contributor

@laurencei laurencei commented Oct 24, 2018

This is to revert PR #26010

Although the PR was merged 16 days ago, it was only "tagged" 18 hours ago. We already have 3x reported issues of people experiencing file system permissions issues, which all of them mention that reverting to 5.7.9 solves their problems: #26230, #26229 and at the bottom of the PR #26010

Arguably the problem is not the PR itself, but incorrect file permissions on existing projects.

The issue is as more people start running composer update we are likely to see an increase in reported tickets. And these problems might not appear until the application is pushed to production, which is even worse.

So I suggest the PR perhaps should be targetted to 5.8 and listed as part of the upgrade checklist.

Of course, reverting the PR re-introduces the original edge case bug that it solved - but that seemed to be less of an issue. πŸ€·β€β™‚οΈ

p.s. I couldnt create an automatic PR to "revert" the changes, as there have been formatting changes since. So if people could cross check this PR with the original to make sure it's a correct revert - that would be helpful.

Copy link
Contributor

@mfn mfn left a comment

Choose a reason for hiding this comment

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

as there have been formatting changes since

This was the change im \Illuminate\Foundation\PackageManifest::write I think which LGTM πŸ‘

@Yogarine
Copy link
Contributor

I identified the exact issue (with help of @Pillmayr) and made a fix here: #26232

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.

None yet

4 participants