Mitigate dependency upon ConfigCache #7753

mpdude · 2013-04-21T07:31:01Z

Q	A
Bug fix?	no
New feature?	yes (refactoring)
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR	todo, issue is symfony/symfony-docs#2531

Some clients use ConfigCache only for its ability to atomically write into a file.

The PR moves that code into the Filesystem component.

It's a pick off #7230.

To-Do:

Update docs for the Filesystem component

mpdude · 2013-04-21T07:38:01Z

Travis exited with a segfault on PHP 5.3.3 only...?!?

jakzal · 2013-04-21T07:53:14Z

@mpdude yes, it's not your fault though. The problem exist for some time now (see #7754).

fabpot · 2013-04-21T07:59:09Z

Thanks for working on smaller PRs. That's much appreciated, and it definitely makes things easier.

Unfortunately, this one cannot be merged as it would introduce a hard dependency between HttpKernel and Config for instance, which we must avoid as some projects using Symfony do not want to depend on Config (like Drupal for instance).

As a rule of thumb, code duplication in Symfony exists for a reason, so refactoring like this one will almost always be rejected.

mpdude · 2013-04-21T09:18:41Z

I was aware of that need of f. e. Drupal, but somehow failed to see the new dep here. Sorry.

Better now?

fabpot · 2013-04-21T09:26:33Z

What about moving CacheFileUtils::dumpInFile() to the Filsystem component class instead? And remove the other two methods that are not used anyway.

The only client of these methods was removed in 669898b.

mpdude · 2013-04-21T10:28:36Z

Regarding the Filesystem component: Good idea and better place, but would be a change dependency-wise? Component/Config then would need Component/Filesystem. Bundle/FrameworkBundle/* probably don't care...

fabpot · 2013-04-21T10:33:05Z

Adding the Filesystem component as a dep to Config is not a problem.

fabpot · 2013-04-21T10:34:14Z

For the method name in Filesystem, we should probably come up with a better name than dumpInFile. dumpFile or storeFile should be enough, no?

mpdude · 2013-04-21T10:41:32Z

Great, yes, will do.

Clients of ConfigCache are/were using 0666 & ~umask().

mpdude · 2013-04-21T12:19:14Z

src/Symfony/Component/Filesystem/Filesystem.php

+        }
+
+        $this->rename($tmpFile, $filename);
+        $this->chmod($filename, $mode);


This is how it was before, but wouldn't it be better to chmod() first before moving the file in place?

fabpot · 2013-04-21T13:21:04Z

Except for the getMetaFile method, everything else looks good to me. Can you also add a note in the Filesystem CHANGELOG?

mpdude · 2013-04-21T13:42:29Z

Adding the getMetaFile() was a reflex action as the meta file name is built in two different methods, though I forgot one of the places when picking from #7230. Shall I make it private?

mpdude · 2013-04-21T14:10:51Z

Just to be sure, don't we have to do anything about the new Config -> Filesystem dep? Add it to the Config component composer.json maybe?

fabpot · 2013-04-21T14:28:11Z

You are right, you need to add the filesystem component as a required dep for the config component.

fabpot · 2013-04-21T14:29:16Z

Nervermind, I'm doing to do it after the merge.

mpdude · 2013-04-21T14:36:26Z

Thanks, learned a lot already today.

The next picks will probably become much more interesting than that :-)

This PR was merged into the master branch. Discussion ---------- [Filesystem] Added a missing test case for Filesystem::dumpFile() This PR adds a test case for the ``Filesystem::dumpFile()`` method (introduced in #7753). | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | [![Build Status](https://travis-ci.org/jakzal/symfony.png?branch=dumpfile-tests)](https://travis-ci.org/jakzal/symfony) | Fixed tickets | - | License | MIT | Doc PR | - Commits ------- 8fcf4c2 [Filesystem] Added a missing test case for Filesystem::dumpFile().

This PR was merged into the master branch. Discussion ---------- [Filesystem] Made sure Filesystem::dumpFile() overwrites an existing file #7753 introduced a change in behaviour. Before, ConfigCache simply used the ``rename()`` function to save a file. It was replaced by ``Filesystem::dumpFile()`` which internally uses ``Filesystem::rename()``. The later checks if file already exists and throws an exception if it exists. This PR makes sure that ``Filesystem::dumpFile()`` removes the file before creating a new one. Alternatively, we could introduce a third parameter to the ``Filesystem::rename()`` which would allow to overwrite the target file. Before #7752: * [CompilerDebugDumpPass](https://github.com/symfony/symfony/blob/d100ffaf761b46acdb2440c04602378b87b0d610/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/CompilerDebugDumpPass.php#L23) uses ConfigCache * [ConfigCache](https://github.com/symfony/symfony/blob/d100ffaf761b46acdb2440c04602378b87b0d610/src/Symfony/Component/Config/ConfigCache.php#L103) uses the [rename()](http://uk1.php.net/rename) function (which overwrites a file if it exists) After #7752: * [CompilerDebugDumpPass](https://github.com/symfony/symfony/blob/ad47bc47380188041b7889f40e380ad3766a0110/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/CompilerDebugDumpPass.php#L23) uses the ``Filesystem::dumpFile()`` * [Filesystem::dumpFile()](https://github.com/webfactory/symfony/blob/ad47bc47380188041b7889f40e380ad3766a0110/src/Symfony/Component/Filesystem/Filesystem.php#L458) uses the [Filesystem::rename()](https://github.com/webfactory/symfony/blob/ad47bc47380188041b7889f40e380ad3766a0110/src/Symfony/Component/Filesystem/Filesystem.php#L239) (which throws an exception if file exists) | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #7816 | License | MIT | Doc PR | ~ Commits ------- 4f4ec76 [Filesystem] Made sure Filesystem::dumpFile() overwrites an existing file.

wouterj · 2013-05-03T11:23:49Z

src/Symfony/Component/Filesystem/CHANGELOG.md

+2.3.0
+-----
+
+ * added the dumpFile() method to atomically write files


typo automatically

No, thats "atomically" (sic) - writing the file in a way that other user see either the old or the new one, but never a partially-written state.

ok, thanks! I've learned a new word :)
Could you please review symfony/symfony-docs#2592 , after your comment I'm not sure if it's correct.

Introduce the CacheFileUtils helper

30a1e32

Revert newly introduced dependency

669898b

Remove unused methods.

ed861ba

The only client of these methods was removed in 669898b.

mpdude added 3 commits April 21, 2013 13:38

Move the remaining method from CacheFileUtils to the Filesystem class

bbb884b

Bug introduced when turning the condition around

249abb3

With dumpFile() in the Filesystem class, the mode must be a param.

327befe

Clients of ConfigCache are/were using 0666 & ~umask().

mpdude reviewed Apr 21, 2013
View reviewed changes

mpdude added 2 commits April 21, 2013 15:39

Added entry for new dumpFile() method

a490486

Use method here as well

6da1b0a

mpdude mentioned this pull request Apr 21, 2013

Document the new dumpFile() method in the Filesystem component symfony/symfony-docs#2531

Closed

Make method private + fix CS #fail

ad47bc4

fabpot closed this in c3756eb Apr 21, 2013

jakzal mentioned this pull request Apr 22, 2013

[Filesystem] Added a missing test case for Filesystem::dumpFile() #7801

Merged

This was referenced Apr 23, 2013

Cannot rename because the target ...DebugProjectContainer.xml already exist #7816

Closed

[Filesystem] Made sure Filesystem::dumpFile() overwrites an existing file #7820

Merged

wouterj mentioned this pull request Apr 29, 2013

[2.3] Document Filesystem::dumpFile method symfony/symfony-docs#2573

Closed

wouterj reviewed May 3, 2013
View reviewed changes

wouterj mentioned this pull request May 3, 2013

Documented dumpFile symfony/symfony-docs#2592

Merged

mpdude deleted the issue-7230/picks/cache-file-utils branch September 24, 2015 19:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mitigate dependency upon ConfigCache #7753

Mitigate dependency upon ConfigCache #7753

mpdude commented Apr 21, 2013

mpdude commented Apr 21, 2013

jakzal commented Apr 21, 2013

fabpot commented Apr 21, 2013

mpdude commented Apr 21, 2013

fabpot commented Apr 21, 2013

mpdude commented Apr 21, 2013

fabpot commented Apr 21, 2013

fabpot commented Apr 21, 2013

mpdude commented Apr 21, 2013

mpdude Apr 21, 2013

fabpot commented Apr 21, 2013

mpdude commented Apr 21, 2013

mpdude commented Apr 21, 2013

fabpot commented Apr 21, 2013

fabpot commented Apr 21, 2013

mpdude commented Apr 21, 2013

wouterj May 3, 2013

mpdude May 3, 2013

wouterj May 3, 2013

Mitigate dependency upon ConfigCache #7753

Mitigate dependency upon ConfigCache #7753

Conversation

mpdude commented Apr 21, 2013

mpdude commented Apr 21, 2013

jakzal commented Apr 21, 2013

fabpot commented Apr 21, 2013

mpdude commented Apr 21, 2013

fabpot commented Apr 21, 2013

mpdude commented Apr 21, 2013

fabpot commented Apr 21, 2013

fabpot commented Apr 21, 2013

mpdude commented Apr 21, 2013

mpdude Apr 21, 2013

Choose a reason for hiding this comment

fabpot commented Apr 21, 2013

mpdude commented Apr 21, 2013

mpdude commented Apr 21, 2013

fabpot commented Apr 21, 2013

fabpot commented Apr 21, 2013

mpdude commented Apr 21, 2013

wouterj May 3, 2013

Choose a reason for hiding this comment

mpdude May 3, 2013

Choose a reason for hiding this comment

wouterj May 3, 2013

Choose a reason for hiding this comment