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

Don't touch files #1542

Closed
wants to merge 3 commits into from
Closed

Don't touch files #1542

wants to merge 3 commits into from

Conversation

barryvdh
Copy link
Contributor

Fixes #1540

Q A
Bug fix? Yes
New feature? No
BC breaks? Yes or No
Deprecations? No
Fixed tickets #1540

Removes the touch so that non-existing files are not created, possibly in an invalid format.

@antonmedv
Copy link
Member

I'm afraid what this is breaking change. How can we figure this out?

@staabm
Copy link
Contributor

staabm commented Feb 13, 2018

I have the same feeling... this might be a breaking change... maybe this should be feature-flagged instead of beeing deleted, so you can opt-out from this touch call in your recipe.

@antonmedv
Copy link
Member

I think we need to see history of this addition and why it was introduced in first place.

@barryvdh
Copy link
Contributor Author

Yeah I was trying to, but at some time there was a large refactor so the history was hard to track down.

The symlink would still be created right? Just as a broken link, which (imho) is easier to notice then an empty file.

@gharlan
Copy link
Contributor

gharlan commented Feb 13, 2018

I think we need to see history of this addition and why it was introduced in first place.

Apparently it was introduced along with whole deploy:shared task in 2014:
339baed#diff-17bfdc9b1a5d2375e8f0241c0bee0619R84

@barryvdh
Copy link
Contributor Author

I guess we could also show a warning message when this doesn't exist, or fail.

But imho it seems to be more dangerous/unexpected to create an empty file, because some frameworks might check for the existence of a file and then require it or change behavior.

@antonmedv
Copy link
Member

@gharlan your are right. Actually shared_dirs also created. What about them?

I think this is kind of breaking changes. I think to add this to v7 milestone.

@antonmedv antonmedv added this to the 7.0 milestone Feb 14, 2018
@barryvdh
Copy link
Contributor Author

So another use-case about this:
Magento has a 'maintenance' flag in 'var/.maintenance.flag'. If that exists, it's in maintenance mode, otherwise not. Currently you can't share that file, because if it doesn't exist you create it, so Magento thinks it's in maintenance mode..

@antonmedv
Copy link
Member

Agree. Will be better to not create file. What about shared dirs?

@cafferata
Copy link
Contributor

If you ask me, I would answer; no shared directory creation and an showing an warning message is enough. Because this situation will only occurs when you bootstrap the project. In our situation the shared directories contains an .gitkeep file to prevent from not existing.

@jordisala1991
Copy link
Contributor

What about a flag to disable/enable this feature... and disabled by default, because otherwise it would be BC break

@antonmedv
Copy link
Member

I think one more flag is redundant. Will merge in into v7 major release.

@antonmedv antonmedv closed this Apr 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Required to touch existing files?
6 participants