Update libraries/joomla/table/table.php #1713

Merged
merged 2 commits into from Mar 18, 2013

Conversation

Projects
None yet
6 participants
Contributor

artur-stepien commented Nov 23, 2012

JTable:reset() should also reset errors array. If they are not reset JTable::getErrors() will return old errors, from previous record.

@artur-stepien artur-stepien Update libraries/joomla/table/table.php
JTable:reset() should also reset errors array. If they are not reset JTable::getErrors() will return old errors, from previous record.
7bf74d9
Contributor

LouisLandry commented Nov 23, 2012

Completely agree with this. Will leave it open a little while to see if we have dissenting opinions worth a discussion.

@eddieajau eddieajau commented on an outdated diff Nov 23, 2012

libraries/joomla/table/table.php
@@ -519,6 +519,8 @@ public function reset()
$this->$k = $v->Default;
}
}
+ // Reset table errors
@eddieajau

eddieajau Nov 23, 2012

Contributor

Need a new line before the comment otherwise the code style checks will complain.

@eddieajau

eddieajau Nov 28, 2012

Contributor

@Malkowitch if you can make this small fix we can get this merged. Thanks in advance.

Contributor

eddieajau commented Nov 23, 2012

+1 from me.

Contributor

artur-stepien commented Nov 28, 2012

I got view more questions:
Why primary key isn't cleared? This function when, I saw it first time looked like it is what I searched for. I wanted to reset all JTable fields so could insert another record without creating another instance.What is the purpose to reset everything without primary keys?

Should this stay like it is now? Should reset() clear all fields? Or there should be another function called for example JTable::clear() that will clear all fields to its default state (with primary keys).

Also there is performance difference with using getInstance() and reset():

 0.000 seconds (+0.000); 8.18 MB (+8.179) - before start:
 0.279 seconds (+0.279); 8.28 MB (+0.102) - After creating 10,000 instances:
 0.279 seconds (+0.000); 8.28 MB (-0.003) - before start:
 0.454 seconds (+0.175); 8.28 MB (+0.003) - After resetting table 10,000 times:

When it comes to loading record 10,000 times, reset() is not needed. But when user want to insert records using
JTable he will have to recreate instance what leads to performance drop.

Is this function anywhere used in current version of Joomla! CMS?

I thing that the best solution is to leave reset() like in this commit, and create another function clear() that will clear all fields including primary keys.

@elinw elinw commented on the diff Dec 2, 2012

libraries/joomla/table/table.php
@@ -519,6 +519,9 @@ public function reset()
$this->$k = $v->Default;
}
}
+
@elinw

elinw Dec 2, 2012

Contributor

jenkins says there is some white space here.

Contributor

ianmacl commented Jan 26, 2013

I know it's been a while, but if you can clean up the one code style violation we'll get this merged in.

Contributor

eddieajau commented Mar 16, 2013

There is 1 code style error in this PR. @Malkowitch would you be able to fix this over the weekend so we can merge it? Thanks in advance.

Contributor

artur-stepien commented Mar 18, 2013

I'm really new to github and don't use it often. There is new line before comment. Is it wrong?

Contributor

dongilbert commented Mar 18, 2013

I'll get this wrapped up for you. It's just some indenting space at the end of a line.

dongilbert merged commit caeab11 into joomla:staging Mar 18, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment