Split piwik.js in multiple files minor tweak #10449

mattab · 2016-08-28T23:43:11Z

mattab · 2016-08-28T23:44:37Z

plugins/CustomPiwikJs/File.php

@@ -60,7 +60,7 @@ public function getName()
     */
    public function hasWriteAccess()
    {
-        return $this->hasReadAccess() && is_writable($this->file);
+        return is_writable($this->file);


hopefully it's fine to just check whether file is writable. It solves the issue when the target file does not exist yet, hasWriteAccess should return true when the file can be created/written to.

FYI: I had it like this on purpose for now and in tests just created the file in advance

tsteur · 2016-08-30T22:36:42Z

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 · 2016-09-19T04:34:38Z

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

…_put_contents($targetFile, '');

mattab · 2016-09-19T04:44:46Z

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

tsteur · 2016-09-19T10:59:30Z

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 · 2016-09-19T19:54:34Z

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 · 2016-09-26T21:28:18Z

@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 · 2016-09-26T21:37:42Z

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 · 2016-09-26T21:54:04Z

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 · 2016-09-27T01:32:50Z

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

…writable and file does not exist yet (#10576) * followup #10449 * Fix test (cherry picked from commit fac3d63) * Prevent chmod(): No such file or directory

* Fix depraction test: use assertDeprecatedMethodIsRemovedInPiwik3 * Fix Scheduled Reports sent one hour late in daylight saving timezones (#10443) * convert hour to send report to/from UTC, to ensure it isn't affected by daylight savings * adds update script to change existing scheduled reports to use utc time * code improvement * adds missing param * Added new event Archiving.makeNewArchiverObject to allow customising plugin archiving (#10366) * added hook to alllow plugin archiving prevention * cr code style notes * reworked PR to fit CR suggestions * added PHPDoc for hook * Event description more consistent * UI tests: minor changes * Comment out Visitor Log UI tests refs #10536 * Adds test checking if all screenshots are stored in lfs * removed screenshots not stored in lfs * readds screenshots to lfs * 2.16.3-b4 * Issue translation updates against 2.x-dev * language update * Fix bug in widget list remove where the JSON object becomes array * 2.16.3-rc1 * console command custom-piwik-js:update should work when directory is writable and file does not exist yet (#10576) * followup #10449 * Fix test (cherry picked from commit fac3d63) * Prevent chmod(): No such file or directory * Automatically update all marketplace plugins when updating Piwik (#10527) * update plugins and piwik at the same time * make sure plugins are updated with piwik * use only one try/catch * reload plugin information once it has been installed * make sure to clear caches after an update * fix ui tests * make sure to use correct php version without any extras * Additional informations passed in the hook "isExcludedVisit" (issue #10415) (#10564) * Additional informations passed in the hook "isExcludedVisit" (issue #10415) * Added better description to the new parameters * Update VisitExcluded.php * Remove two parameters not needed as better to use the Request object * Update VisitExcluded.php * remove extra two parameters in VisitExcluded constructor to prevent confusion (#10593) * Update our libs to latest #10526 * Update composer libraries to latest #10526 * Update log analytics to latest * When updating the config file failed (or when any other file is not writable...), the Updater (for core or plugins) will now automatically throw an error and cancel the update (#10423) * When updating the config file failed (or when any other file is not writable...), the Updater (for core or plugins) will now automatically throw an error and cancel the update * add integration test to check the correct exception is thrown when config not writabel * New integration test for updater * Make test better * When opening the visitor profile, do not apply the segment (#10533) * When opening the visitor profile, do not apply the segment * added ui test for profile but does work for me * next try to make ui test work * add expected screenshot * added missing doc

Split piwik.js in multiple files minor tweak

107a707

mattab added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Aug 28, 2016

mattab added this to the 2.16.3 milestone Aug 28, 2016

mattab reviewed Aug 28, 2016
View reviewed changes

tsteur changed the base branch from master to 2.x-dev September 1, 2016 06:36

Do not create the piwik.js in tests as it's not needed

76fa28d

check whether test failure is related to previous commit: re-add file…

f7b75b4

…_put_contents($targetFile, '');

mattab merged commit 0e57896 into 2.x-dev Sep 19, 2016

mattab deleted the 10441b branch September 19, 2016 04:45

mattab added a commit that referenced this pull request Sep 27, 2016

followup #10449

84386cf

mattab mentioned this pull request Sep 27, 2016

console command custom-piwik-js:update should work when directory is writable and file does not exist yet #10576

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split piwik.js in multiple files minor tweak #10449

Split piwik.js in multiple files minor tweak #10449

mattab commented Aug 28, 2016

mattab Aug 28, 2016

tsteur Aug 29, 2016

tsteur commented Aug 30, 2016

mattab commented Sep 19, 2016

mattab commented Sep 19, 2016

tsteur commented Sep 19, 2016

tsteur commented Sep 19, 2016

mattab commented Sep 26, 2016

tsteur commented Sep 26, 2016

mattab commented Sep 26, 2016

mattab commented Sep 27, 2016

Split piwik.js in multiple files minor tweak #10449

Split piwik.js in multiple files minor tweak #10449

Conversation

mattab commented Aug 28, 2016

mattab Aug 28, 2016

Choose a reason for hiding this comment

tsteur Aug 29, 2016

Choose a reason for hiding this comment

tsteur commented Aug 30, 2016

mattab commented Sep 19, 2016

mattab commented Sep 19, 2016

tsteur commented Sep 19, 2016

tsteur commented Sep 19, 2016

mattab commented Sep 26, 2016

tsteur commented Sep 26, 2016

mattab commented Sep 26, 2016

mattab commented Sep 27, 2016