[Koha-bugs] [Bug 14321] Merge UploadedFile and UploadedFiles into Koha::Upload

[bugzilla-daemon at bugs.koha-community.org]

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14321

--- Comment #50 from Marcel de Rooy <m.de.rooy at rijksmuseum.nl> ---
Jonathan: Thanks for looking into this one!

> +sub DESTROY {
> +}
> This is not really useful.
Agreed. Removing it.

> +# **************  HELPER ROUTINES / CLASS METHODS
> Should be part of POD.
Right!

> +sub _fh {
> I don't understand the usefulness of this method.
Hm. I am not sure anymore, but this routine may have had some more lines in
development. But it still contains a test if (a) then a->b, so I want to keep
it in order to not repeat this test a few times. Keeps the code more readable
at the calling spots.

> +        $self->{files}->{$filename}->{errcode} = 3; #no rootdir
> Please avoid that and use readable error code. "no_rootdir" would be perfect.
I hope that is not a blocker for you :) and prefer to leave it as-is now..

> +        $p= $rec->{permanent}? $self->{rootdir}: $self->{tmpdir};
> +        $p.= '/';
> +        $p.= $rec->{dir}. '/'. $rec->{hashvalue}. '_'. $rec->{filename};
> It would be better to use File::Spec::catfile
OK. Adjusting it.

> +    my $i = $dbh->last_insert_id(undef, undef, 'uploaded_files', undef);
> last_insert_id is mysql specific.
> It would have been better to create the Koha::UploadFile[s] based on 
> Koha::Object[s] to avoid and write sql queries.
Could be true, but that is a too large rewrite operation at this moment.
We could do that on a new report at some point in time. Some day we hope to not
have anything mysql specific anymore?

> +sub _lookup {
> +    my ( $self, $params ) = @_;
> +    my $dbh = C4::Context->dbh;
> +    my $sql = qq|
> Variable interpolations should not be allowed in SQL queries.
Which line do you exactly refer to now?
Probably the WHERE id IN ($params->{id}) line later on.
Note that the id variable is only allowed to be of the form 1,2,3,4 by the
regex directly preceding it. If you persist here, I could rebuild it in the
form ?,?,? here and first count the number of id's..

Will continue with your second comment now.

-- 
You are receiving this mail because:
You are watching all bug changes.