Bug #117019 for File-Path: File::Path::remove_tree option hash is auto populated and cannot be reused for a second call without warnings

Wed Aug 17 13:32:04 2016 me [...] eboxr.com - Ticket created

Subject:

File::Path::remove_tree option hash is auto populated and cannot be reused for a second call without warnings

File::Path::remove_tree (and maybe more) is using the options hashref passed internally and populate it with noise... You cannot set an hashref for options and reuse it for another call as it s going to be polluted and you will noticed some warning Wonder if it s an expected behavior or if the hashref should be localized... Note that when using version 2.09 this could be used without warnings... whereas after the update to 2.12 now this is adding a warning (which is a good thing) for unknown options mkdir -p /tmp/a/b/c; perl -w -MFile::Path -E 'use warnings; my $x= ""; my $opts = { error => \$x }; File::Path::remove_tree("/tmp/a/b/c", $opts ); File::Path::remove_tree("/tmp/a/b", $opts ); say $File::Path::VERSION; use Test::More; note explain $opts' Unrecognized option(s) passed to remove_tree(): cwd depth device inode perm prefix at -e line 1. 2.1201 # { # 'cwd' => '/usr/local/cpanel', # 'depth' => 0, # 'device' => 64769, # 'error' => \[], # 'inode' => 68670914, # 'perm' => undef, # 'prefix' => '' # }

Wed Aug 17 17:10:23 2016 jkeenan [...] cpan.org - Correspondence added

On Wed Aug 17 13:32:04 2016, atoomic wrote: Show quoted text

> File::Path::remove_tree (and maybe more) is using the options hashref > passed internally and populate it with noise... > You cannot set an hashref for options and reuse it for another call as > it s going to be polluted and you will noticed some warning >

Agreed. It appears that the %options is being used -- correctly -- as a way to store modifications to the function call, and incorrectly as a way to store metadata or data about the results of the same function call. Bad design -- but with File::Path that's nothing new. Your friendly neighborhood File::Path maintainers have shaken their heads many times over such problems. And, unfortunately, when we've tried to rectify such problems, we're very prone to complaints from users who, for example, don't want to see new warnings. There are workarounds if you are really, really bothered by this problem. See attachment. In a second invocation of 'remove_tree', you can either have the 'error' element in the options hash point to a *different* scalar variable, or you can purge the options hash of all elements except those which you want and which are documented to work. Show quoted text

> Wonder if it s an expected behavior or if the hashref should be > localized... > Note that when using version 2.09 this could be used without > warnings... whereas after the update to 2.12 now this is adding a > warning (which is a good thing) for unknown options > > mkdir -p /tmp/a/b/c; perl -w -MFile::Path -E 'use warnings; my $x= ""; > my $opts = { error => \$x }; File::Path::remove_tree("/tmp/a/b/c", > $opts ); File::Path::remove_tree("/tmp/a/b", $opts ); say > $File::Path::VERSION; use Test::More; note explain $opts' > Unrecognized option(s) passed to remove_tree(): cwd depth device inode > perm prefix at -e line 1. > 2.1201 > # { > # 'cwd' => '/usr/local/cpanel', > # 'depth' => 0, > # 'device' => 64769, > # 'error' => \[], > # 'inode' => 68670914, > # 'perm' => undef, > # 'prefix' => '' > # }

I suspect that simply documenting the problem will be vastly easier than trying to "fix" it and keep CPAN working. Thank you very much. Jim Keenan

Subject:

117019-opts.pl

# perl use strict; use warnings; use 5.10.1; use Carp; use File::Path; use Test::More qw( no_plan ); use File::Temp qw( tempdir ); my ($opts, $tdir, $tdir2, $tdir3, $tdir4); my ($x, $y, $z) = ("") x 3; $opts = { verbose => 1, error => \$x }; $tdir = tempdir( CLEANUP => 1 ); ok( -d $tdir, "tempdir $tdir created"); File::Path::remove_tree($tdir, $opts ); ok(! (-d $tdir), "tempdir $tdir removed, as expected"); note explain $opts; $y = ""; $opts = { verbose => 1, error => \$y }; $tdir2 = tempdir( CLEANUP => 1 ); ok( -d $tdir2, "tempdir $tdir2 created"); File::Path::remove_tree($tdir2, $opts ); ok(! (-d $tdir2), "tempdir $tdir2 removed, as expected"); note explain $opts; $z = ""; $opts = { verbose => 1, error => \$z }; $tdir3 = tempdir( CLEANUP => 1 ); ok( -d $tdir3, "tempdir $tdir3 created"); File::Path::remove_tree($tdir3, $opts ); ok(! (-d $tdir3), "tempdir $tdir3 removed, as expected"); $tdir4 = tempdir( CLEANUP => 1 ); ok( -d $tdir4, "tempdir $tdir4 created"); my @opts_for_removal = grep { ! m/^(error|verbose)$/ } keys %{$opts}; delete @{$opts}{ @opts_for_removal }; File::Path::remove_tree($tdir4, $opts ); ok(! (-d $tdir4), "tempdir $tdir4 removed, as expected"); note explain $opts; say "Finished";

Wed Aug 17 17:10:24 2016 The RT System itself - Status changed from 'new' to 'open'

Thu Aug 18 11:51:09 2016 me [...] eboxr.com - Correspondence added

Indeed I'm good with a documentation only change, so at least people will not (or will be aware to not) reuse the same hashref. I've just converted my $opts = { ... }; to a simple sub opts { return {...} }; so I'm now using a new hashref each time... But I'm curious how many modules on CPAN are using it this way (outside of unit test) and this might raise unexpected warnings in production when updating File::Path on several products/systems... On Wed Aug 17 17:10:23 2016, JKEENAN wrote: Show quoted text

> On Wed Aug 17 13:32:04 2016, atoomic wrote:

> > File::Path::remove_tree (and maybe more) is using the options hashref > > passed internally and populate it with noise... > > You cannot set an hashref for options and reuse it for another call > > as > > it s going to be polluted and you will noticed some warning > >

> > Agreed. It appears that the %options is being used -- correctly -- as > a way to store modifications to the function call, and incorrectly as > a way to store metadata or data about the results of the same function > call. > > Bad design -- but with File::Path that's nothing new. Your friendly > neighborhood File::Path maintainers have shaken their heads many times > over such problems. And, unfortunately, when we've tried to rectify > such problems, we're very prone to complaints from users who, for > example, don't want to see new warnings. > > There are workarounds if you are really, really bothered by this > problem. See attachment. In a second invocation of 'remove_tree', > you can either have the 'error' element in the options hash point to a > *different* scalar variable, or you can purge the options hash of all > elements except those which you want and which are documented to work. >

> > Wonder if it s an expected behavior or if the hashref should be > > localized... > > Note that when using version 2.09 this could be used without > > warnings... whereas after the update to 2.12 now this is adding a > > warning (which is a good thing) for unknown options > > > > mkdir -p /tmp/a/b/c; perl -w -MFile::Path -E 'use warnings; my $x= > > ""; > > my $opts = { error => \$x }; File::Path::remove_tree("/tmp/a/b/c", > > $opts ); File::Path::remove_tree("/tmp/a/b", $opts ); say > > $File::Path::VERSION; use Test::More; note explain $opts' > > Unrecognized option(s) passed to remove_tree(): cwd depth device > > inode > > perm prefix at -e line 1. > > 2.1201 > > # { > > # 'cwd' => '/usr/local/cpanel', > > # 'depth' => 0, > > # 'device' => 64769, > > # 'error' => \[], > > # 'inode' => 68670914, > > # 'perm' => undef, > > # 'prefix' => '' > > # }

> > I suspect that simply documenting the problem will be vastly easier > than trying to "fix" it and keep CPAN working. > > Thank you very much. > Jim Keenan

Wed Mar 08 17:42:41 2017 jkeenan [...] cpan.org - Correspondence added

On Thu Aug 18 11:51:09 2016, atoomic wrote: Show quoted text

> Indeed I'm good with a documentation only change, so at least people > will not (or will be aware to not) reuse the same hashref. > I've just converted my $opts = { ... }; to a simple sub opts { return > {...} }; so I'm now using a new hashref each time... > > But I'm curious how many modules on CPAN are using it this way > (outside of unit test) > and this might raise unexpected warnings in production when updating > File::Path on several products/systems... > > On Wed Aug 17 17:10:23 2016, JKEENAN wrote:

> > On Wed Aug 17 13:32:04 2016, atoomic wrote:

> > > File::Path::remove_tree (and maybe more) is using the options > > > hashref > > > passed internally and populate it with noise... > > > You cannot set an hashref for options and reuse it for another call > > > as > > > it s going to be polluted and you will noticed some warning > > >

> > > > Agreed. It appears that the %options is being used -- correctly -- > > as > > a way to store modifications to the function call, and incorrectly as > > a way to store metadata or data about the results of the same > > function > > call. > > > > Bad design -- but with File::Path that's nothing new. Your friendly > > neighborhood File::Path maintainers have shaken their heads many > > times > > over such problems. And, unfortunately, when we've tried to rectify > > such problems, we're very prone to complaints from users who, for > > example, don't want to see new warnings. > > > > There are workarounds if you are really, really bothered by this > > problem. See attachment. In a second invocation of 'remove_tree', > > you can either have the 'error' element in the options hash point to > > a > > *different* scalar variable, or you can purge the options hash of all > > elements except those which you want and which are documented to > > work. > >

> > > Wonder if it s an expected behavior or if the hashref should be > > > localized... > > > Note that when using version 2.09 this could be used without > > > warnings... whereas after the update to 2.12 now this is adding a > > > warning (which is a good thing) for unknown options > > > > > > mkdir -p /tmp/a/b/c; perl -w -MFile::Path -E 'use warnings; my $x= > > > ""; > > > my $opts = { error => \$x }; File::Path::remove_tree("/tmp/a/b/c", > > > $opts ); File::Path::remove_tree("/tmp/a/b", $opts ); say > > > $File::Path::VERSION; use Test::More; note explain $opts' > > > Unrecognized option(s) passed to remove_tree(): cwd depth device > > > inode > > > perm prefix at -e line 1. > > > 2.1201 > > > # { > > > # 'cwd' => '/usr/local/cpanel', > > > # 'depth' => 0, > > > # 'device' => 64769, > > > # 'error' => \[], > > > # 'inode' => 68670914, > > > # 'perm' => undef, > > > # 'prefix' => '' > > > # }

> > > > I suspect that simply documenting the problem will be vastly easier > > than trying to "fix" it and keep CPAN working. > > > > Thank you very much. > > Jim Keenan

Would you be able to try out the version of lib/File/Path.pm found here: https://github.com/jkeenan/File-Path/blob/fc457bb14afb8203258ede6fe1e40516a09db4b5/lib/File/Path.pm This modifies remove_tree() -- but not make_path()! -- to internally separate the hashref holding options passed to the subroutine from the hashref holding those options (once verified) and internally generated parameters. No documentation updates yet. Thank you very much. Jim Keenan

Fri Mar 10 09:28:59 2017 jkeenan [...] cpan.org - Correspondence added

On Wed Mar 08 17:42:41 2017, JKEENAN wrote: Show quoted text

> On Thu Aug 18 11:51:09 2016, atoomic wrote:

> > Indeed I'm good with a documentation only change, so at least people > > will not (or will be aware to not) reuse the same hashref. > > I've just converted my $opts = { ... }; to a simple sub opts { return > > {...} }; so I'm now using a new hashref each time... > > > > But I'm curious how many modules on CPAN are using it this way > > (outside of unit test) > > and this might raise unexpected warnings in production when updating > > File::Path on several products/systems... > > > > On Wed Aug 17 17:10:23 2016, JKEENAN wrote:

> > > On Wed Aug 17 13:32:04 2016, atoomic wrote:

> > > > File::Path::remove_tree (and maybe more) is using the options > > > > hashref > > > > passed internally and populate it with noise... > > > > You cannot set an hashref for options and reuse it for another > > > > call > > > > as > > > > it s going to be polluted and you will noticed some warning > > > >

> > > > > > Agreed. It appears that the %options is being used -- correctly -- > > > as > > > a way to store modifications to the function call, and incorrectly > > > as > > > a way to store metadata or data about the results of the same > > > function > > > call. > > > > > > Bad design -- but with File::Path that's nothing new. Your > > > friendly > > > neighborhood File::Path maintainers have shaken their heads many > > > times > > > over such problems. And, unfortunately, when we've tried to > > > rectify > > > such problems, we're very prone to complaints from users who, for > > > example, don't want to see new warnings. > > > > > > There are workarounds if you are really, really bothered by this > > > problem. See attachment. In a second invocation of 'remove_tree', > > > you can either have the 'error' element in the options hash point > > > to > > > a > > > *different* scalar variable, or you can purge the options hash of > > > all > > > elements except those which you want and which are documented to > > > work. > > >

> > > > Wonder if it s an expected behavior or if the hashref should be > > > > localized... > > > > Note that when using version 2.09 this could be used without > > > > warnings... whereas after the update to 2.12 now this is adding a > > > > warning (which is a good thing) for unknown options > > > > > > > > mkdir -p /tmp/a/b/c; perl -w -MFile::Path -E 'use warnings; my > > > > $x= > > > > ""; > > > > my $opts = { error => \$x }; > > > > File::Path::remove_tree("/tmp/a/b/c", > > > > $opts ); File::Path::remove_tree("/tmp/a/b", $opts ); say > > > > $File::Path::VERSION; use Test::More; note explain $opts' > > > > Unrecognized option(s) passed to remove_tree(): cwd depth device > > > > inode > > > > perm prefix at -e line 1. > > > > 2.1201 > > > > # { > > > > # 'cwd' => '/usr/local/cpanel', > > > > # 'depth' => 0, > > > > # 'device' => 64769, > > > > # 'error' => \[], > > > > # 'inode' => 68670914, > > > > # 'perm' => undef, > > > > # 'prefix' => '' > > > > # }

> > > > > > I suspect that simply documenting the problem will be vastly easier > > > than trying to "fix" it and keep CPAN working. > > > > > > Thank you very much. > > > Jim Keenan

> > Would you be able to try out the version of lib/File/Path.pm found > here: > > https://github.com/jkeenan/File- > Path/blob/fc457bb14afb8203258ede6fe1e40516a09db4b5/lib/File/Path.pm > > This modifies remove_tree() -- but not make_path()! -- to internally > separate the hashref holding options passed to the subroutine from the > hashref holding those options (once verified) and internally generated > parameters. >

In branch, I have now modified mkpath() to make the same internal distinction between key-value pairs passed in via an options hash and those internally generated. However, it turns out that we were not at risk for kind of warning you got with remove_tree() because all of the valid keys for an options hash for make_path() are assigned internally. Show quoted text

> No documentation updates yet. >

I didn't see the need to change any documentation. These changes are all now reflected in:: https://github.com/rpcme/File-Path/pull/41 Thank you very much. Jim Keenan

Wed May 31 20:50:52 2017 jkeenan [...] cpan.org - Correspondence added

On Fri Mar 10 09:28:59 2017, JKEENAN wrote: Show quoted text

> On Wed Mar 08 17:42:41 2017, JKEENAN wrote:

> > On Thu Aug 18 11:51:09 2016, atoomic wrote:

> > > Indeed I'm good with a documentation only change, so at least > > > people > > > will not (or will be aware to not) reuse the same hashref. > > > I've just converted my $opts = { ... }; to a simple sub opts { > > > return > > > {...} }; so I'm now using a new hashref each time... > > > > > > But I'm curious how many modules on CPAN are using it this way > > > (outside of unit test) > > > and this might raise unexpected warnings in production when > > > updating > > > File::Path on several products/systems... > > > > > > On Wed Aug 17 17:10:23 2016, JKEENAN wrote:

> > > > On Wed Aug 17 13:32:04 2016, atoomic wrote:

> > > > > File::Path::remove_tree (and maybe more) is using the options > > > > > hashref > > > > > passed internally and populate it with noise... > > > > > You cannot set an hashref for options and reuse it for another > > > > > call > > > > > as > > > > > it s going to be polluted and you will noticed some warning > > > > >

> > > > > > > > Agreed. It appears that the %options is being used -- correctly > > > > -- > > > > as > > > > a way to store modifications to the function call, and > > > > incorrectly > > > > as > > > > a way to store metadata or data about the results of the same > > > > function > > > > call. > > > > > > > > Bad design -- but with File::Path that's nothing new. Your > > > > friendly > > > > neighborhood File::Path maintainers have shaken their heads many > > > > times > > > > over such problems. And, unfortunately, when we've tried to > > > > rectify > > > > such problems, we're very prone to complaints from users who, for > > > > example, don't want to see new warnings. > > > > > > > > There are workarounds if you are really, really bothered by this > > > > problem. See attachment. In a second invocation of > > > > 'remove_tree', > > > > you can either have the 'error' element in the options hash point > > > > to > > > > a > > > > *different* scalar variable, or you can purge the options hash of > > > > all > > > > elements except those which you want and which are documented to > > > > work. > > > >

> > > > > Wonder if it s an expected behavior or if the hashref should be > > > > > localized... > > > > > Note that when using version 2.09 this could be used without > > > > > warnings... whereas after the update to 2.12 now this is adding > > > > > a > > > > > warning (which is a good thing) for unknown options > > > > > > > > > > mkdir -p /tmp/a/b/c; perl -w -MFile::Path -E 'use warnings; my > > > > > $x= > > > > > ""; > > > > > my $opts = { error => \$x }; > > > > > File::Path::remove_tree("/tmp/a/b/c", > > > > > $opts ); File::Path::remove_tree("/tmp/a/b", $opts ); say > > > > > $File::Path::VERSION; use Test::More; note explain $opts' > > > > > Unrecognized option(s) passed to remove_tree(): cwd depth > > > > > device > > > > > inode > > > > > perm prefix at -e line 1. > > > > > 2.1201 > > > > > # { > > > > > # 'cwd' => '/usr/local/cpanel', > > > > > # 'depth' => 0, > > > > > # 'device' => 64769, > > > > > # 'error' => \[], > > > > > # 'inode' => 68670914, > > > > > # 'perm' => undef, > > > > > # 'prefix' => '' > > > > > # }

> > > > > > > > I suspect that simply documenting the problem will be vastly > > > > easier > > > > than trying to "fix" it and keep CPAN working. > > > > > > > > Thank you very much. > > > > Jim Keenan

> > > > Would you be able to try out the version of lib/File/Path.pm found > > here: > > > > https://github.com/jkeenan/File- > > Path/blob/fc457bb14afb8203258ede6fe1e40516a09db4b5/lib/File/Path.pm > > > > This modifies remove_tree() -- but not make_path()! -- to internally > > separate the hashref holding options passed to the subroutine from > > the > > hashref holding those options (once verified) and internally > > generated > > parameters. > >

> > In branch, I have now modified mkpath() to make the same internal > distinction between key-value pairs passed in via an options hash and > those internally generated. However, it turns out that we were not at > risk for kind of warning you got with remove_tree() because all of the > valid keys for an options hash for make_path() are assigned > internally. >

> > No documentation updates yet. > >

> > I didn't see the need to change any documentation. > > These changes are all now reflected in:: > > https://github.com/rpcme/File-Path/pull/41 > > Thank you very much. > Jim Keenan

The content of p.r. 41 was merged into master in March. File-Path version 2.13 was released to CPAN today (May 31 2017). Marking this ticket Resolved. Thank you very much. Jim Keenan

Wed May 31 20:50:58 2017 jkeenan [...] cpan.org - Status changed from 'open' to 'resolved'

Wed May 31 20:51:08 2017 jkeenan [...] cpan.org - Taken