Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DataObject API duplication on ->exists() and ->isInDB() #9352

Closed
patricknelson opened this issue Dec 11, 2019 · 1 comment · Fixed by #10407
Closed

DataObject API duplication on ->exists() and ->isInDB() #9352

patricknelson opened this issue Dec 11, 2019 · 1 comment · Fixed by #10407

Comments

@patricknelson
Copy link
Contributor

patricknelson commented Dec 11, 2019

Affected Version

All versions of SilverStripe (up to 4.4.4 at time of writing).

Description

Currently we have two methods that do almost exactly the same thing (both semantically and functionally). My opinion is currently that we should either:

  • Simply point ->exists() to ->isInDB() since, at the DataObject level, it exists if it is also in the DB. For files, this degree of differentiation may make sense because the file can potentially still be in the database but no longer "exist" on the file system (or on some object store) or vice versa.
  • Deprecate (and ultimately remove) ->isInDB() since it is redundant at the DataObject level (as it can only exist in the database). This of course discards the notion of "existing" as an ephemeral object in memory which hasn't yet had a ->write() call, of course (since that is implicit). If this is done, it should just be added on classes lower down where it makes sense to differentiate between the two.

IMHO, the method ->exists() should be a canonical method of determining that an object is properly present in all it's necessary forms. For example, with simple data this method will ensure it's in the database (regardless of how that is actually determined, e.g. simple ID check or actually hitting the DB with a query to verify). For files, this should also presumably look at storage to validate existence.

Also, another argument for this is that these two methods (at least in DataObject) are nearly identical:

https://github.com/silverstripe/silverstripe-framework/blob/4.4.4/src/ORM/DataObject.php#L759-L762

    public function exists()
    {
        return (isset($this->record['ID']) && $this->record['ID'] > 0);
    }

https://github.com/silverstripe/silverstripe-framework/blob/4.4.4/src/ORM/DataObject.php#L3802-L3805

    public function isInDB()
    {
        return is_numeric($this->ID) && $this->ID > 0;
    }

I'd like to hash this out since I want to standardize my code on a method like ->exists() instead of doing a direct check of $obj->ID > 0 since that would likely be impacted if we ever moved to a UUID-based ID system like those discussed or proposed in #6819 and #8411.

PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants