@mattab opened this Pull Request on August 28th 2016 Member

refs #10441

@tsteur commented on August 30th 2016 Member

We now need to update the tests that create this file in advance and also tests/javascript/index.php which creates this file in advance too file_put_contents($targetFile, '');

@mattab commented on September 19th 2016 Member

After removing the file_put_contents the JavaScript tests fail with

Executing tests in test suite JavascriptTests...

Phantom callbacks not attached in time.  See http://github.com/mark-rushakoff/OpenPhantomScripts/issues/1

Unsafe JavaScript attempt to access frame with URL about:blank from frame with URL file://../javascript/testrunner.js. Domains, protocols and ports must match.

I'm wondering whether it's caused by my change or not. I'll try to re-add the file_put_contents again and see

@mattab commented on September 19th 2016 Member

Works if I leave the file_put_contents so I'll leave it for now...

@tsteur commented on September 19th 2016 Member

If it works when you leave the file_put_contents, this PR (https://github.com/piwik/piwik/pull/10449/files#diff-296a183c286a6fe5a8c5d1ae4ecea1d3L63) seems to not 100% work

@tsteur commented on September 19th 2016 Member

Be good to add a test for this as I don't think it works. I think there might be even a test that would have failed otherwise if it worked so no test would have needed but not sure

@mattab commented on September 26th 2016 Member

@tsteur I'm a bit confused here... can't think of a case where it does not work. willing to leave it as is unless you point me to the issue (hope i'm not lazy)

@tsteur commented on September 26th 2016 Member

If your PR worked it would fail eg https://github.com/piwik/piwik/blob/2.x-dev/plugins/CustomPiwikJs/tests/Integration/FileTest.php#L69

See http://us2.php.net/manual/en/function.is-writable.php is_writable only returns true when file exists and is writable. What you want is probably is_writable(dirname($this->file)) but recursive up in case the directory is missing but one of the other directories is missing and it needs to create the directories when calling save(). Eg it might be /home/test/foo/bar/baz in this case it needs to first test /home/test/foo/bar/, then /home/test/foo/ then /home/test/foo/ and on ->save() create missing directories

@mattab commented on September 26th 2016 Member

If your PR worked it would fail eg https://github.com/piwik/piwik/blob/2.x-dev/plugins/CustomPiwikJs/tests/Integration/FileTest.php#L69

This test should only fail when the NOT_EXISTING_FILE's directory is not writable. on travis and locally the directory is writable so even though file doesn't exist, it's still writable and can be created by the console command. that's actually the reason I removed the is_readable: so that when the directory is writable but the file doesn't exist yet, the console won't complain. Am I missing something?

@mattab commented on September 27th 2016 Member

Ok now it's clear, thanks, see the PR that fixes the issue https://github.com/piwik/piwik/pull/10576/files

This Pull Request was closed on September 19th 2016
Powered by GitHub Issue Mirror