[RTM] Improve model registry #7725

Toflar · 2015-03-26T19:30:30Z

This is my PR to solve #7724 and even more!

Currently, our model registry only allows to register models by their PK which in general is id. This causes useless DB queries in a lot of circumstances. The one mentioned in #7724 was tl_files.path which is actually unique as well and currently when executing FilesModel::findByPath('path') 20 times, there will be 20 queries to the database even though it's always the same.

Note that this happens quite often since Contao 3.4, because our Image class does that for every image to check if an important part is set.

Now instead of just solving this issue I tried to find a general solution for those cases :-)
The model registry can now handle aliases that internally point to the ID again.
Let's say we have this in tl_files:

id	path
42	path_to_my_file.jpg

In Contao 3.4 AND my PR (= so it's fully backwards compatible) you can fetch the model from the registry via its ID:

Model\Registry::getInstance()->fetch('tl_files', 42);

With this PR you can now also do this:

Model\Registry::getInstance()->fetch('tl_files', 'path_to_my_file.jpg', 'path');

And thanks to the modifications on the DcaExtractor this automatically works for all fields that are unique either because of the SQL key definition:

'sql' => array
(
    'keys' => array
    (
        'uuid' => 'unique'
    )
)

or the eval section of the field itself:

'path' => array
(
    'eval'                    => array('unique'=>true),
    'sql'                     => "varchar(1022) NOT NULL default ''",
),

Every developer can either do that or override the getUniqueFields() method in their model class for full flexibility.

🎉

…r eval definition)

…f the findOneBy() method

leofeyer · 2015-03-26T19:31:40Z

system/modules/core/dca/tl_files.php

@@ -146,7 +146,8 @@
 		),
 		'path' => array
 		(
-			'sql'                     => "varchar(1022) NOT NULL default ''"
+			'eval'                    => array('unique'=>true),


This is wrong, the path must not be unique.

??? That's what the whole PR and discussion today was about?? 20x FilesModel::findByPath('path.jpg') should return the same instance because it's unique? If not, what combination of columns does make it unique then?

Only the UUID is unique. If you e.g. use SyncCto it may happen that you have two or more entries of the same file with different UUIDs, therefore we decided not to make the path unique.

"unique" in terms of "unique for this installation". Everywhere we use findByPath() we expect a model, not a collection (if it was not unique it would return a collection, right?).
See e.g.
https://github.com/contao/core/blob/develop/system/modules/core/controllers/BackendPopup.php#L96
https://github.com/contao/core/blob/develop/system/modules/core/forms/FormFileUpload.php#L253
https://github.com/contao/core/blob/develop/system/modules/core/library/Contao/Picture.php#L123
https://github.com/contao/core/blob/develop/system/modules/core/modules/ModuleRegistration.php#L431

leofeyer · 2015-03-26T19:32:33Z

I have added the [RFC] tag to the title of the PR, assuming that it no longer is work in progress?

leofeyer · 2015-03-27T10:05:41Z

I really cannot merge it because of the path column not being unique. The discussion about it is here: #5556

leofeyer · 2015-03-27T10:12:44Z

After having read #5556 again, I'm no longer sure if the column is not unique after all. It seems we just weren't able to set a unique index on DB level.

Toflar · 2015-03-27T10:20:40Z

My changes do NOT set any keys :) It's just our eval->unique feature which in DC_Table is checked here: https://github.com/contao/core/blob/master/system/modules/core/drivers/DC_Table.php#L2855
It's not even in use for DC_File so it won't have any effect at all when editing files in the back end. The only thing it does is, it makes sure that 20 FilesModel::findByPath('foobar.jpg') calls all return the same model and do not execute 20 queries.

leofeyer · 2015-03-27T10:29:55Z

Ok, so we must never set a unique index on tl_files.path or tl_files.pid and tl_files.path combined. It will throw an exception if the file paths exceed 250 characters (as discussed in #5556 and verified again in a test right now). It does, however, work with a regular btree index.

This means that we can actually merge the PR, because the column is meant to be unique (we just cannot set an index on DB level).

Toflar · 2015-03-27T10:34:28Z

Okay, but please do not merge it yet. I would like to improve some naming things and try if can support arbitrary aliases that do not necessarily have to match 1 column only (so in theory it would be possible to alias multiple columns).

leofeyer · 2015-03-27T10:47:14Z

I won't. The title still says [RFC] and not [RTM] :)

leofeyer · 2015-03-27T15:48:54Z

system/modules/core/library/Contao/DcaExtractor.php

@@ -479,6 +512,11 @@ protected function createExtract()
 			foreach ($sql['keys'] as $field=>$type)
 			{
 				$this->arrKeys[$field] = $type;
+
+				if ($type === 'unique')


We are only using == in the legacy code.

…does not necessarily have to be a column name)

…self so every model can control registering and unregistering aliases itself

Toflar · 2015-03-30T10:31:07Z

So there's some more work done here but leaving on RFC because I want to explain what I did here.
Now every Model has the possibility to do things with the registry in the onRegister() and onUnregister() methods/callbacks. By default those ones add aliases for unique fields to the registry, so you don't have to do anything here if you want this feature. However, now you can optimize your own models to now execute the query again.
Let's say you have something like this in your model:

public static function findOneByExpensiveFilterQueryMethod(array $criteria)
{
    // Build complex query
    return static::findOneBy($queryParts);
}

Now you still have the issue that you might call this method with the exact same $criteria array and thus the query gets executed 20 times although your models have actually been loaded before.

You can now optimize like this:

public static function findOneByExpensiveFilterQueryMethod(array $criteria)
{
    $alias = 'my_superb_alias';
    // Of course you may need to order the criteria here or so etc.
    $aliasValue = md5($criteria);

    $model = \Model\Registry::getInstance()->fetch(static::getTable(), $aliasValue, $alias);

    if ($model !== null) {
        return $model;
    }

    // Build complex query

    $model = static::findOneBy($queryParts);

    if ($model !== null) {
        \Model\Registry::getInstance()->registerAlias($model, $alias, $aliasValue);
        return $model;
    }

    return null;
}

Or use the onRegister() and onUnregister() callbacks etc. Lots of possibilities, now :)

leofeyer · 2015-03-30T10:33:32Z

system/modules/core/library/Contao/Model.php

+		foreach (static::getUniqueFields() as $strColumn)
+		{
+			$varAliasValue = $this->{$strColumn};
+			if (!$registry->isAliasRegistered($this, $strColumn, $varAliasValue))


Please rename this method to isRegisteredAlias().

Done in 3c7e12c.

leofeyer · 2015-03-30T10:35:58Z

I like it. Can I merge it now?

Toflar · 2015-03-30T10:36:52Z

If none of the @contao/developers has any objections, yes :)

leofeyer · 2015-03-30T10:38:43Z

I am in the process of releasing the beta version of Contao 3.5. How long shall I wait?

Toflar · 2015-03-30T10:39:26Z

Okay for me if this gets a bit more time to be reviewed by the other devs and makes it into the RC later in April :)

leofeyer · 2015-03-30T10:42:08Z

👍

discordier · 2015-03-30T11:14:22Z

system/modules/core/library/Contao/Model.php

+	{
+		$objDca = \DcaExtractor::getInstance(static::getTable());
+
+		if ($objDca->hasUniqueFields())


This can be omitted as \DcaExtractor is initializing to empty array.
Simply always return the value of $objDca->getUniqueFields()

True, „fixed“ in 2337fa6.

…n array

discordier · 2015-03-30T12:11:51Z

system/modules/core/library/Contao/Model.php

@@ -891,13 +937,22 @@ public static function findOneBy($strColumn, $varValue, array $arrOptions=array(
 	 */
 	public static function findBy($strColumn, $varValue, array $arrOptions=array())
 	{
+		$blnModel = false;
+
+		$arrColumn = (array) $strColumn;


The naming of $strColumn is now misleading, so is the phpDoc.
This should be changed to reflect the new behaviour.

Why? I did not change this afaik?

I may be a string or an array in your implementation but was only a string before.
Otherwise the check on count($arrColumn) == 1 is not needed as it will always be 1.

No, it could always be an array.

Then the description was misleading from the beginning. :)

discordier · 2015-03-30T12:26:22Z

system/modules/core/library/Contao/Model/Registry.php

 	{
-		if (isset($this->arrRegistry[$strTable][$intPk]))
+		// Default is searching by PK and is the most common case


I know we are in legacy code here and not in Contao 4 but please please please let's not increase the complexity of the old classes if we can use methods for new code even if it is in legacy classes.

Better in 81bd651?

discordier · 2015-03-30T12:28:22Z

Added all of my remarks.

leofeyer · 2015-04-07T06:49:53Z

@Toflar Is this RTM now?

Toflar · 2015-04-07T06:58:31Z

Yes. :)

leofeyer · 2015-04-07T06:59:04Z

Great!

leofeyer · 2015-04-07T08:03:37Z

Can you please take a look at the following two FIXMEs?

core/system/modules/core/library/Contao/Model.php

Line 1041 in ce8e824

// FIXME: what about line 1089?

core/system/modules/core/library/Contao/Model.php

Line 1053 in ce8e824

    
           return $objModel; // FIXME: in line 1099, we are returning $objModel->mergeRow($objResult->row());

Toflar · 2015-04-07T08:11:38Z

To be honest, I don't know why we're using mergeRow() there because if a model can be found in the registry you want to have this one back not an instance merged with DB data? This would mean that it's possible that the model in the registry does not contain the same data as in the DB row which would render the whole registry obsolete :) So imho the first FIXME can be ignored because it's correct to return the model instance from the registry if it's available and the second FIXME is invalid in terms of it's correct to check the registry again because my new code is before the query and the old and after the query. So it might be that the new code cannot find a model in the registry without executing the query (if this does not match:

core/system/modules/core/library/Contao/Model.php

Line 1046 in ce8e824

    
           if (count($arrColumn) == 1 && ($arrColumn[0] == static::$strPk || in_array($arrColumn[0], static::getUniqueFields())))

) but after the query is being executed it's still possible that it returns a result that is already in the registry so it's not necessary to instantiate that object again. However that mergeRow() call there still looks wrong to me as this would modify a model in the registry.

leofeyer · 2015-04-07T08:20:51Z

What about line 1048? Is $arrOptions['value'] or $arrOptions['value'][0] always $intId? Or should we rename $intId to $varKey?

Toflar · 2015-04-07T08:36:20Z

$varKey would be more appropriate because while in most cases it's an integer (if you call findByPk(42)), it will be a string for e.g. the FilesModel if you call findByPath('path/to/file.jpg'), you're right.

leofeyer · 2015-04-07T08:39:43Z

Merged in 9da066c then :)

Toflar added 7 commits March 26, 2015 20:07

DcaExtractor now knows about unique fields (either defined by index o…

866591a

…r eval definition)

Added a model helper method to get the unique fields

df1fad0

Model registry now supports adding aliases

9161802

Added a simple cache for Model::getClassFromTable() calls

44cef38

Model::findBy now already knows if a single model is required

c3eec36

Try to fetch a model from the registry in the find() method instead o…

a101dd1

…f the findOneBy() method

tl_files.path is unique

2e87555

Toflar mentioned this pull request Mar 26, 2015

Improve FilesModel::findByPath() #7724

Closed

leofeyer added the feature label Mar 26, 2015

leofeyer added this to the 3.5.0 milestone Mar 26, 2015

leofeyer reviewed Mar 26, 2015
View reviewed changes

leofeyer changed the title ~~Improve model registry~~ [RFC] Improve model registry Mar 26, 2015

leofeyer reviewed Mar 27, 2015
View reviewed changes

Toflar added 4 commits March 30, 2015 11:38

Renamed to alias because you can pass whatever you like as a string (…

b36538e

…does not necessarily have to be a column name)

Added public helper methods for alias handling to the registry

c0b15f4

Delegate the register and unregister process to the model instance it…

6420256

…self so every model can control registering and unregistering aliases itself

Call static what's static

342d728

leofeyer reviewed Mar 30, 2015
View reviewed changes

Renamed isAliasRegistered() to isRegisteredAlias()

3c7e12c

leofeyer self-assigned this Mar 30, 2015

discordier reviewed Mar 30, 2015
View reviewed changes

No need for an additional check as getUniqueFields() always returns a…

2337fa6

…n array

discordier reviewed Mar 30, 2015
View reviewed changes

Simplified Registry::fetch()

5c26d36

discordier reviewed Mar 30, 2015
View reviewed changes

Toflar added 2 commits March 30, 2015 14:32

CS

7032e63

Reduce complexity in Registry::fetch()

81bd651

Toflar changed the title ~~[RFC] Improve model registry~~ [RTM] Improve model registry Apr 7, 2015

leofeyer merged commit 81bd651 into contao:develop Apr 7, 2015

Toflar deleted the feature/improve-model-registry branch April 7, 2015 09:07

[RTM] Improve model registry #7725

[RTM] Improve model registry #7725

Conversation

Toflar commented Mar 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leofeyer commented Mar 26, 2015

leofeyer commented Mar 27, 2015

leofeyer commented Mar 27, 2015

Toflar commented Mar 27, 2015

leofeyer commented Mar 27, 2015

Toflar commented Mar 27, 2015

leofeyer commented Mar 27, 2015

Choose a reason for hiding this comment

Toflar commented Mar 30, 2015

Choose a reason for hiding this comment

Toflar Mar 30, 2015 via email

Choose a reason for hiding this comment

leofeyer commented Mar 30, 2015

Toflar commented Mar 30, 2015

leofeyer commented Mar 30, 2015

Toflar commented Mar 30, 2015

leofeyer commented Mar 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Toflar Mar 30, 2015 via email

Choose a reason for hiding this comment

discordier commented Mar 30, 2015

leofeyer commented Apr 7, 2015

Toflar commented Apr 7, 2015

leofeyer commented Apr 7, 2015

leofeyer commented Apr 7, 2015

Toflar commented Apr 7, 2015

leofeyer commented Apr 7, 2015

Toflar commented Apr 7, 2015

leofeyer commented Apr 7, 2015