Feature Request #2538

ORM::__set should account for values which haven't changed

Added by Kent Davidson over 4 years ago. Updated over 4 years ago.

Status:FeedbackStart date:02/04/2010
Priority:NormalDue date:
Assignee:Isaiah DeRose-Wilson% Done:

100%

Category:-
Target version:2.1
Resolution: Points:

Description

It's convenient to know whether a database object has been modified locally, regardless of whether or not changes to it are identical. Very useful when synchronizing a database table using ORM with an external source.

For example, doing:

$user = ORM::factory('user');
$user->email = "[email protected]";

When $user->email is already "" causes ORM::$changed to be updated, incorrectly, in my opinion.

Suggestion is to change 2nd if block of ORM::__set should be changed to:

            $new_value = $this->load_type($column, $value);

            if (!isset($this->object[$column]) || $this->object[$column] !== $new_value)
            {
                if (isset($this->table_columns[$column]))
                {
                    // Data has changed
                    $this->changed[$column] = $column;

                    // Object is no longer saved
                    $this->saved = FALSE;
                }

                $this->object[$column] = $new_value;
            }

This will prevent "changed" from being modified when it really hasn't changed.

ORM.php Magnifier - ORM.php from v.2.3.4 (32.9 KB) Kent Davidson, 02/04/2010 04:42 AM

History

#1 Updated by Isaiah DeRose-Wilson over 4 years ago

  • Project changed from Kohana v2.x to ORM - K2
  • Target version deleted (2.3.4)

#2 Updated by Isaiah DeRose-Wilson over 4 years ago

  • Target version set to 2.0.1

#3 Updated by Isaiah DeRose-Wilson over 4 years ago

I'm not sure this is a good idea. Comparing strings with == (or !=) generally isn't a good idea. Consider the following:

$current_value = "0123";
$new_value = 123;
var_dump($current_value != $new_value);

With your patch you would get unexpected results as the above example demonstrates. I'm going to leave this issue open and get some feedback from the other developers, however I have a feeling the unintended consequences of this patch aren't worth the advantages.

#4 Updated by Kent Davidson over 4 years ago

I'm not sure if you misread the code, but I'm comparing with !== and ===, and as well, the value is type adjusted with $this->load_type which does type conversion based on the database type (albeit rudimentary).

Unfortunately, load_type has just been removed in Kohana v3 so this argument is now moot; however, it should be possible anyway.

I think in terms of performance and "not hitting the database", whatever the application (ORM) can do to prevent unncessary SQL calls is paramount to any web app. A fairly typical example here is when I update my database from an external source (say, for example, a CSV file imported). I do:


$csv_headers = array('name','productId','price','last_updated');
$added = $updated = $total = 0;
while (!feof($f)) {
    $csv_line = fgetcsv($f);
    if (!$csv_line) {
        break;
    }
    $x = ORM::factory("MyObject")->find(array('name' => $csv_line[0]));
    foreach ($csv_line as $i => $value) {
        $member = $csv_headers[$i];
        $x->$member = $value;
    }
    if ($x->changed()) {
        if (!$x->id) {
            ++$added;
        } else 
            ++$updated;
        }
        $x->save();
    }
    ++$total;
}
echo "$added added, $updated updated, $total processed.\n" 

Similarly, I appears that ORM for Kohana v3 is starting to think this way as well:

http://github.com/kohana/orm/commit/34bd3e1f4010d3eb360d8a757ff58ab95827f791

I obviously need to move towards Kohana 3, but this still appears to be missing there as well.

Again, I'll reiterate that the worst case scenario for adding this functionality is an unintentional update when the data hasn't changed. The dramatic improvement will be that, in general, your database will be hit less, and isn't better database performance a great consequence?

Esp. with high-performance web applications, updates kill query caches and ruin performance. I should be able to run the above script and have NO updates go to the database. I can instrument a bunch of external code to check if a value will change an internal value, but IMHO this belongs in the core class.

I'm glad to see that Kohana development remains active; based on what I see on the web sites I was getting worried there:

http://v3.kohanaphp.com

Dummy Lipsum code? As a side note: Where do I post bugs about existing web sites? There are a bunch of bugs in the sites (the above being one) and you could use some usability help as well. I like Kohana, and want to contribute, but you need a project which is the four apps so I can log bugs against them:

http://learn.kohanaphp.com/
http://dev.kohanaphp.com/
http://www.kohanaphp.com/
http://docs.kohanaphp.com/

Thanks.

#5 Updated by Jeremy Bush over 4 years ago

Kent Davidson wrote:

I'm glad to see that Kohana development remains active; based on what I see on the web sites I was getting worried there:

http://v3.kohanaphp.com

This is not a production site in any sense of the word. It's a development playground :)

Dummy Lipsum code? As a side note: Where do I post bugs about existing web sites? There are a bunch of bugs in the sites (the above being one) and you could use some usability help as well. I like Kohana, and want to contribute, but you need a project which is the four apps so I can log bugs against them:

http://learn.kohanaphp.com/

This is a wordpress blog, about to be dropped (it's very old).

http://dev.kohanaphp.com/

This is redmine, not sure how there'd be "bugs" in the issue tracking software, but you'd have to report them to the redmine team.

http://www.kohanaphp.com/

This is getting replaced very soon.

http://docs.kohanaphp.com/

This is dokuwiki, and it's getting replaced very soon. If you find mistakes in the docs, you can login and edit them yourself. It's a wiki.

Thanks.

We'd welcome any help. Right now we are focused on getting documentation done for version 2.4 to get it released and into the wild.

#6 Updated by Kent Davidson over 4 years ago

Before we get too far OT, I like Kohana a lot and want to contribute, but I'm concerned with how thing appear to me (as a newbie). Impressions of technology, as you know, are essential to the success of a project. Since my impressions have swung from "Wow, this this is really well written" to "gee, they have some obvious issues with QA of their sites", and frustration trying to find answers; it just invokes some concern.

I was simply listing the component sites that I've found so far, not saying they all have bugs. I've only seen issues on v3 and forum; and there are some basic suggestions as well for usability where there's no place to log them.

Start with:
- http://forum.kohanaphp.com/termsofservice.php
linked from the signup page being a blank page.

http://v3.kohanaphp.com/ ... is linked from Stack Overflow and other places. To put a notice on there stating it's a playground, and they should visit another site shouldn't take more than a few minutes, and would probably contribute to confidence from other users. Once it's linked from anywhere on the internet, you've gotta assume it's for public consumption; it's certainly indexed in Google already.

I would do the fix, but I don't know if I am able to contribute code without the blessing of the core team.

I've signed up for the four (Dev, Docs, Forum, Github) or more accounts needed to get involved (again, kind of a PITA) and will contribute to the docs and bugbases as needed for now. a

#7 Updated by Isaiah DeRose-Wilson over 4 years ago

  • Status changed from New to Review
  • Assignee set to Isaiah DeRose-Wilson

Kent Davidson wrote:

I'm not sure if you misread the code

Fair point, I did misread your code. I'm going to have to think about this change a bit more, but it might be a useful one.

Let's try to keep this issue on topic, for general site feedback I suggest posting in the forums (please use one of the existing topics though, there has been lots of community feedback about the Kohana website already).

#8 Updated by Isaiah DeRose-Wilson over 4 years ago

  • Status changed from Review to Closed
  • % Done changed from 0 to 100

Applied in changeset r9.

#9 Updated by Isaiah DeRose-Wilson over 4 years ago

  • Status changed from Closed to Review
  • Target version changed from 2.0.1 to 2.0

This shouldn't be closed yet... the fix is still in the experimental branch.

#10 Updated by Isaiah DeRose-Wilson over 4 years ago

  • Status changed from Review to Assigned

#11 Updated by Isaiah DeRose-Wilson over 4 years ago

  • Status changed from Assigned to Feedback

#12 Updated by Isaiah DeRose-Wilson over 4 years ago

  • Target version changed from 2.0 to 2.1

Also available in: Atom PDF