-
Notifications
You must be signed in to change notification settings - Fork 822
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
FIX: Remove some unnecessary ClassInfo calls in DataObjectSchema #7670
FIX: Remove some unnecessary ClassInfo calls in DataObjectSchema #7670
Conversation
530b7cf
to
fb522b0
Compare
src/ORM/DataObjectSchema.php
Outdated
@@ -201,6 +201,11 @@ public function fieldSpecs($classOrInstance, $options = 0) | |||
$db = []; | |||
$classes = $uninherited ? [$class] : ClassInfo::ancestry($class); | |||
foreach ($classes as $tableClass) { | |||
// Skip irrelevant parent classes | |||
if ($tableClass !== DataObject::class && !is_subclass_of($tableClass, DataObject::class)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tractorcow Omitting the $tableClass !== DataObject::class
saves around another 500 method calls. As far as I can tell $this->databaseFields(DataObject::class);
doesn’t return anything, do you think that would be safe?
It looks like the fixed fields will always be forcibly included by cacheDatabaseFields()
anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job. :)
fb522b0
to
4e9cad6
Compare
@@ -394,6 +399,10 @@ public function databaseIndexes($class, $aggregated = true) | |||
*/ | |||
public function classHasTable($class) | |||
{ | |||
if (!is_subclass_of($class, DataObject::class)) { | |||
return false; | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somehow ViewableData
gets passed in here, I haven’t yet tracked down why but this seems like a reasonable change anyway.
It is causing the test failure... normally $schema->classHasTable('NonExistentClass')
would pass the class name over to databaseFields()
and therefore ClassInfo::class_name()
which triggers an exception (instead of returning false like the new behaviour would). We could either remove that assertion, or if that would be deemed an API change replace it with a direct class_exists()
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think removing the assertion is ok... It's kind of stupid IMO, we probably should have been catching it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's from dataClassesFor() not properly filtering is_subclass_of() the way you have been doing (correctly).
This seems reasonable to me - the exception throwing is the only thing we need to take a view on regarding if that's an API issue as people could be relying on it... |
4e9cad6
to
91bd92d
Compare
I’ve removed the test assertion for an exception to be thrown in Also, as this is targeting |
@@ -138,11 +138,11 @@ public function tableName($class) | |||
*/ | |||
public function baseDataClass($class) | |||
{ | |||
$class = ClassInfo::class_name($class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we not add the if (!is_subclass_of($tableClass, DataObject::class))
check here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does handle that at the end of the method, but I suppose we could move it to the start before the while
loop
@@ -394,6 +399,10 @@ public function databaseIndexes($class, $aggregated = true) | |||
*/ | |||
public function classHasTable($class) | |||
{ | |||
if (!is_subclass_of($class, DataObject::class)) { | |||
return false; | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's from dataClassesFor() not properly filtering is_subclass_of() the way you have been doing (correctly).
ClassInfo::ancestry()
will includeViewableData
andDataObject
in its results. Subsequent calls toDataObjectSchema::databaseFields(ViewableData::class)
are a waste of time of course, as there are no database fields on those classes.Below profile is for a homepage load on a blank install, but the performance difference will only grow the more
DataObject
classes are referenced during a given request.