-
-
Notifications
You must be signed in to change notification settings - Fork 213
[RTM] Improve model registry #7725
Changes from 11 commits
866591a
df1fad0
9161802
44cef38
c3eec36
a101dd1
2e87555
b36538e
c0b15f4
6420256
342d728
3c7e12c
2337fa6
5c26d36
7032e63
81bd651
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,6 +65,12 @@ abstract class Model | |
*/ | ||
protected static $strPk = 'id'; | ||
|
||
/** | ||
* Class name cache | ||
* @var array | ||
*/ | ||
protected static $arrClassNames = array(); | ||
|
||
/** | ||
* Data | ||
* @var array | ||
|
@@ -257,6 +263,25 @@ public static function getPk() | |
} | ||
|
||
|
||
/** | ||
* Return an array of unique field/column names | ||
* Do not include the PK here as this is handled separately | ||
* | ||
* @return array | ||
*/ | ||
public static function getUniqueFields() | ||
{ | ||
$objDca = \DcaExtractor::getInstance(static::getTable()); | ||
|
||
if ($objDca->hasUniqueFields()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be omitted as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, „fixed“ in 2337fa6. |
||
{ | ||
return $objDca->getUniqueFields(); | ||
} | ||
|
||
return array(); | ||
} | ||
|
||
|
||
/** | ||
* Return the name of the related table | ||
* | ||
|
@@ -677,6 +702,44 @@ public function attach() | |
} | ||
|
||
|
||
/** | ||
* Called when the model is attached/registered to the model registry | ||
* | ||
* @param \Model\Registry | ||
*/ | ||
public function onRegister(\Model\Registry $registry) | ||
{ | ||
// Register aliases to unique fields | ||
foreach (static::getUniqueFields() as $strColumn) | ||
{ | ||
$varAliasValue = $this->{$strColumn}; | ||
if (!$registry->isAliasRegistered($this, $strColumn, $varAliasValue)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please rename this method to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 3c7e12c.
|
||
{ | ||
$registry->registerAlias($this, $strColumn, $varAliasValue); | ||
} | ||
} | ||
} | ||
|
||
|
||
/** | ||
* Called when the model is detached/unregistered from the model registry | ||
* | ||
* @param \Model\Registry | ||
*/ | ||
public function onUnregister(\Model\Registry $registry) | ||
{ | ||
// Unregister aliases to unique fields | ||
foreach (static::getUniqueFields() as $strColumn) | ||
{ | ||
$varAliasValue = $this->{$strColumn}; | ||
if ($registry->isAliasRegistered($this, $strColumn, $varAliasValue)) | ||
{ | ||
$registry->unregisterAlias($this, $strColumn, $varAliasValue); | ||
} | ||
} | ||
} | ||
|
||
|
||
/** | ||
* Prevent saving the model | ||
* | ||
|
@@ -846,23 +909,6 @@ public static function findMultipleByIds($arrIds, array $arrOptions=array()) | |
*/ | ||
public static function findOneBy($strColumn, $varValue, array $arrOptions=array()) | ||
{ | ||
// Try to load from the registry | ||
if (empty($arrOptions)) | ||
{ | ||
$arrColumn = (array) $strColumn; | ||
|
||
if (count($arrColumn) == 1 && $arrColumn[0] == static::$strPk) | ||
{ | ||
$intId = is_array($varValue) ? $varValue[0] : $varValue; | ||
$objModel = \Model\Registry::getInstance()->fetch(static::$strTable, $intId); | ||
|
||
if ($objModel !== null) | ||
{ | ||
return $objModel; | ||
} | ||
} | ||
} | ||
|
||
$arrOptions = array_merge | ||
( | ||
array | ||
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The naming of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? I did not change this afaik? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I may be a string or an array in your implementation but was only a string before. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, it could always be an array. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then the description was misleading from the beginning. :) |
||
|
||
if (count($arrColumn) == 1 && ($arrColumn[0] === static::getPk() || in_array($arrColumn[0], static::getUniqueFields()))) | ||
{ | ||
$blnModel = true; | ||
} | ||
|
||
$arrOptions = array_merge | ||
( | ||
array | ||
( | ||
'column' => $strColumn, | ||
'value' => $varValue, | ||
'return' => 'Collection' | ||
'return' => $blnModel ? 'Model' : 'Collection' | ||
), | ||
|
||
$arrOptions | ||
|
@@ -988,6 +1043,25 @@ protected static function find(array $arrOptions) | |
return null; | ||
} | ||
|
||
if ($arrOptions['return'] === 'Model') | ||
{ | ||
$arrColumn = (array) $arrOptions['column']; | ||
|
||
if (count($arrColumn) == 1) | ||
{ | ||
if ($arrColumn[0] == static::$strPk || in_array($arrColumn[0], static::getUniqueFields())) | ||
{ | ||
$intId = is_array($arrOptions['value']) ? $arrOptions['value'][0] : $arrOptions['value']; | ||
$objModel = \Model\Registry::getInstance()->fetch(static::$strTable, $intId, $arrColumn[0]); | ||
|
||
if ($objModel !== null) | ||
{ | ||
return $objModel; | ||
} | ||
} | ||
} | ||
} | ||
|
||
$arrOptions['table'] = static::$strTable; | ||
$strQuery = static::buildFindQuery($arrOptions); | ||
|
||
|
@@ -1121,9 +1195,15 @@ public static function countAll() | |
*/ | ||
public static function getClassFromTable($strTable) | ||
{ | ||
if (isset(static::$arrClassNames[$strTable])) | ||
{ | ||
return static::$arrClassNames[$strTable]; | ||
} | ||
|
||
if (isset($GLOBALS['TL_MODELS'][$strTable])) | ||
{ | ||
return $GLOBALS['TL_MODELS'][$strTable]; // see 4796 | ||
static::$arrClassNames[$strTable] = $GLOBALS['TL_MODELS'][$strTable]; // see 4796 | ||
return static::$arrClassNames[$strTable]; | ||
} | ||
else | ||
{ | ||
|
@@ -1134,7 +1214,8 @@ public static function getClassFromTable($strTable) | |
array_shift($arrChunks); | ||
} | ||
|
||
return implode('', array_map('ucfirst', $arrChunks)) . 'Model'; | ||
static::$arrClassNames[$strTable] = implode('', array_map('ucfirst', $arrChunks)) . 'Model'; | ||
return static::$arrClassNames[$strTable]; | ||
} | ||
} | ||
|
||
|
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.
This is wrong, the path must not be unique.
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.
??? 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?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.
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.
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.
"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