Skip to content

Conversation

@butonic
Copy link
Member

@butonic butonic commented Aug 17, 2016

using sth like

foreach ($options as $key => $option) {
  $option[$key] = self::setUserVars(OCP\User::getUser(), $option);
}

will replace all values in the array with a reference to the value. when you later try to copy the array and modify a value (eg to filter out passwords), expecting the original array to be left intact ... you will be disappointed

cc @PVince81 @jvillafanez @DeepDiver1975

will submit PRs up to master

@mention-bot
Copy link

@butonic, thanks for your PR! By analyzing the annotation information on this pull request, we identified @PVince81, @Xenopathic and @icewind1991 to be potential reviewers

@butonic
Copy link
Member Author

butonic commented Aug 17, 2016

marking as regression because without this wnd will faul to work when logging is enabled because we merged the trace logging. actually only the test on the admin page fails ... oh whatever

@butonic butonic changed the title remove reference magic [stable9] remove reference magic Aug 17, 2016
@PVince81
Copy link
Contributor

do you think this could cause other regressions like, other twisted code that would expect this twisted situation ?

@butonic
Copy link
Member Author

butonic commented Aug 17, 2016

actually I am more worried what the current code does when sending the configreport. The reference magic I am removing is only used to shorten the code ...

@butonic butonic force-pushed the stable9_remove_reference_magic branch from a3d8b8e to 2aa0c34 Compare August 17, 2016 15:34
@PVince81
Copy link
Contributor

Tested with SFTP with $user var in the root path, still works fine 👍

@jvillafanez
Copy link
Member

jvillafanez commented Aug 17, 2016

If it's not commented, I'd expect this to work this way 👍

@butonic butonic force-pushed the stable9_remove_reference_magic branch from 2aa0c34 to d751859 Compare August 18, 2016 08:03
@butonic butonic force-pushed the stable9_remove_reference_magic branch from d751859 to 643c803 Compare August 18, 2016 08:48
@PVince81
Copy link
Contributor

Retested, still works

@PVince81 PVince81 mentioned this pull request Aug 18, 2016
@PVince81
Copy link
Contributor

this wasn't discovered by CI and likely won't either.

A local run of ./autotest-external.sh which covers some cases didn't show any failures.

± % ./autotest-external.sh sqlite sftp-atmoz
Using database oc_autotest
Setup environment for sqlite testing ...
cp: cannot stat ‘/srv/www/htdocs/owncloud/tests/autoconfig-sqlite.php’: No such file or directory
Installing ....
The process control (PCNTL) extensions are required in case you want to interrupt long running commands - see http://php.net/manual/en/book.pcntl.php
ownCloud is not installed - only a limited number of commands are available
creating sqlite db
ownCloud was successfully installed
Testing with sqlite ...
Run only sftp-atmoz ...
The process control (PCNTL) extensions are required in case you want to interrupt long running commands - see http://php.net/manual/en/book.pcntl.php
files_external enabled
No coverage
PHPUnit 4.8.23 by Sebastian Bergmann and contributors.

Runtime:        PHP 5.5.14 with Xdebug 2.4.0
Configuration:  /srv/www/htdocs/owncloud/tests/phpunit-autotest-external.xml

...............................................................  63 / 186 ( 33%)
............................................................... 126 / 186 ( 67%)
............................................................

Time: 1.82 seconds, Memory: 32.00Mb

OK (186 tests, 682 assertions)

@PVince81 PVince81 merged commit 3306473 into stable9 Aug 18, 2016
@PVince81 PVince81 deleted the stable9_remove_reference_magic branch August 18, 2016 09:52
@lock
Copy link

lock bot commented Aug 4, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants