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.

Sign up for GitHub

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

Jump to bottom

WIP Saying good bye to *_where_sql() functions #11407

Merged
merged 3 commits into from Nov 29, 2017
Merged

WIP Saying good bye to *_where_sql() functions #11407

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
921dabc
chore(db): removes remaining raw SQL functions
hypeJunction Nov 26, 2017
ef1d358
chore(tests): move view rendering tests to unit suite
hypeJunction Nov 26, 2017
29e44ac
chore(db): add boolean casting to property pair normalization
hypeJunction Nov 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions docs/appendix/upgrade-notes/2.x-to-3.0.rst
View file
Open in desktop
Expand Up @@ -190,6 +190,9 @@ Deprecated APIs
* ``ban_user``: Use ``ElggUser->ban()``
* ``create_metadata``: Use ``ElggEntity`` setter or ``ElggEntity->setMetadata()``
* ``update_metadata``: Use ``ElggMetadata->save()``
* ``get_metadata_url``
* ``create_annotation``: Use ``ElggEntity->annotate()``
* ``update_metadata``: Use ``ElggAnnotation->save()``
* ``elgg_get_user_validation_status``: Use ``ElggUser->isValidated()``
* ``make_user_admin``: Use ``ElggUser->makeAdmin()``
* ``remove_user_admin``: Use ``ElggUser->removeAdmin()``
Expand All @@ -203,6 +206,7 @@ Deprecated APIs
* ``elgg_list_entities_from_relationship``: Use ``elgg_list_entities()``
* ``elgg_list_entities_from_private_settings``: Use ``elgg_list_entities()``
* ``elgg_list_entities_from_access_id``: Use ``elgg_list_entities()``
* ``elgg_batch_delete_callback``
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why remove this function? It can be helpful sometimes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it creates a anti-pattern. It makes it harder to read the code when used with ElggBatch. I'd rather we write explicit code with object instances that are type hinted.


Removed global vars
-------------------
Expand Down Expand Up @@ -281,6 +285,11 @@ an instance of ``\Elgg\Database\QueryBuilder`` and return a prepared statement.
Plugins should not rely on joined and selected table aliases. Closures passed to the options array will receive a second argument
that corresponds to the selected table alias. Plugins must perform their own joins and use joined aliases accordingly.

Note that all of the private API around building raw SQL strings has also been removed. If you were relying on them in your plugins,
be advised that anything marked as ``@access private`` and ``@internal`` in core can be modified and removed at any time, and we do not guarantee
any backward compatibility for those functions. DO NOT USE THEM. If you find yourself needing to use them, open an issue
on Github and we will consider adding a public equivalent.

Metadata Changes
----------------

Expand Down Expand Up @@ -546,6 +555,7 @@ Miscellaneous API changes
* The config value ``entity_types`` is no longer present or used.
* Uploaded images are autorotated based on their orientation metadata.
* The view ``object/widget/edit/num_display`` now uses an ``input/number`` field instead of ``input/select``; you might need to update your widget edit views accordingly.
* Annotation names are no longer trimmed during save

View extension behaviour changed
--------------------------------
Expand Down
2 changes: 1 addition & 1 deletion engine/classes/Elgg/BootData.php
View file
Open in desktop
Expand Up @@ -23,7 +23,7 @@ class BootData {
/**
* @var \ElggPlugin[]
*/
private $active_plugins = [];
private $active_plugins;

/**
* @var array
Expand Down
86 changes: 56 additions & 30 deletions engine/classes/Elgg/Cache/MetadataCache.php
View file
Open in desktop
Expand Up @@ -19,9 +19,9 @@ class MetadataCache {
protected $values = [];

/**
* @var \ElggSession
* @var array
*/
protected $session;
protected $ids = [];

/**
* @var ElggSharedMemoryCache
Expand All @@ -47,12 +47,14 @@ public function __construct(ElggSharedMemoryCache $cache = null) {
*
* @param int $entity_guid The GUID of the entity
* @param array $values The metadata values to cache
* @param array $ids The metadata ids
* @return void
*
* @access private For testing only
*/
public function inject($entity_guid, array $values) {
public function inject($entity_guid, array $values, array $ids = []) {
$this->values[$entity_guid] = $values;
$this->ids[$entity_guid] = $ids;
}

/**
Expand All @@ -77,6 +79,28 @@ public function getSingle($entity_guid, $name) {
}
}

/**
* Get the metadata id for a particular name
*
* Warning: You should always call isLoaded() beforehand to verify that this
* function's return value can be trusted.
*
* @see isLoaded
*
* @param int $entity_guid The GUID of the entity
* @param string $name The metadata name
*
* @return int[]|int|null
*/
public function getSingleId($entity_guid, $name) {
if (isset($this->ids[$entity_guid])
&& array_key_exists($name, $this->ids[$entity_guid])) {
return $this->ids[$entity_guid][$name];
} else {
return null;
}
}

/**
* Forget about all metadata for an entity.
*
Expand All @@ -85,6 +109,7 @@ public function getSingle($entity_guid, $name) {
*/
public function clear($entity_guid) {
unset($this->values[$entity_guid]);
unset($this->ids[$entity_guid]);
$this->cache->delete($entity_guid);
}

Expand All @@ -95,7 +120,7 @@ public function clear($entity_guid) {
* @return bool
*/
public function isLoaded($entity_guid) {
return array_key_exists($entity_guid, $this->values);
return array_key_exists($entity_guid, $this->values) && array_key_exists($entity_guid, $this->ids);
}

/**
Expand All @@ -108,6 +133,7 @@ public function clearAll() {
$this->cache->delete($guid);
}
$this->values = [];
$this->ids = [];
}

/**
Expand All @@ -120,8 +146,10 @@ public function clearAll() {
public function invalidateByOptions(array $options) {
if (empty($options['guid'])) {
$this->clearAll();
_elgg_services()->entityCache->clear();
} else {
$this->clear($options['guid']);
_elgg_services()->entityCache->remove($options['guid']);
}
}

Expand All @@ -135,17 +163,14 @@ public function populateFromEntities($guids) {
if (empty($guids)) {
return;
}

$version = (int) _elgg_config()->version;
if (!empty($version) && ($version < 2016110900)) {
// can't use this during upgrade from 2.x to 3.0
return;
}

if (!is_array($guids)) {
$guids = [$guids];
}
$guids = array_unique($guids);
$guids = array_unique((array) $guids);

foreach ($guids as $i => $guid) {
$value = $this->cache->load($guid);
Expand All @@ -167,42 +192,43 @@ public function populateFromEntities($guids) {
'callback' => false,
'distinct' => false,
'order_by' => [
new OrderByClause('n_table.entity_guid'),
new OrderByClause('n_table.time_created'),
new OrderByClause('n_table.id'),
new OrderByClause('n_table.entity_guid', 'asc'),
new OrderByClause('n_table.time_created', 'asc'),
new OrderByClause('n_table.id', 'asc')
],
];

// We already have a loaded entity, so we can ignore entity access clauses
$ia = elgg_set_ignore_access(true);
$data = _elgg_services()->metadataTable->getAll($options);
elgg_set_ignore_access($ia);

// make sure we show all entities as loaded
foreach ($guids as $guid) {
$this->values[$guid] = null;
$this->ids[$guid] = null;
}

// build up metadata for each entity, save when GUID changes (or data ends)
$last_guid = null;
$metadata = [];
$last_row_idx = count($data) - 1;
foreach ($data as $i => $row) {
$id = $row->id;
$name = $row->name;
$value = ($row->value_type === 'text') ? $row->value : (int) $row->value;
$guid = $row->entity_guid;
if ($guid !== $last_guid) {
if ($last_guid) {
$this->values[$last_guid] = $metadata;
}
$metadata = [];
}
if (isset($metadata[$name])) {
$metadata[$name] = (array) $metadata[$name];
$metadata[$name][] = $value;

if (isset($this->values[$guid][$name])) {
$this->values[$guid][$name] = (array) $this->values[$guid][$name];
$this->values[$guid][$name][] = $value;
} else {
$metadata[$name] = $value;
$this->values[$guid][$name] = $value;
}
if (($i == $last_row_idx)) {
$this->values[$guid] = $metadata;

if (isset($this->ids[$guid][$name])) {
$this->ids[$guid][$name] = (array) $this->ids[$guid][$name];
$this->ids[$guid][$name][] = $id;
} else {
$this->ids[$guid][$name] = $id;
}
$last_guid = $guid;
}

foreach ($guids as $guid) {
Expand All @@ -220,7 +246,7 @@ public function populateFromEntities($guids) {
* @return array
*/
public function filterMetadataHeavyEntities(array $guids, $limit = 1024000) {

$data = _elgg_services()->metadataTable->getAll([
'guids' => $guids,
'limit' => 0,
Expand All @@ -229,7 +255,7 @@ public function filterMetadataHeavyEntities(array $guids, $limit = 1024000) {
'order_by' => 'n_table.entity_guid, n_table.time_created ASC',
'group_by' => 'n_table.entity_guid',
]);

// don't cache if metadata for entity is over 10MB (or rolled INT)
foreach ($data as $row) {
if ($row->bytes > $limit || $row->bytes < 0) {
Expand Down
127 changes: 0 additions & 127 deletions engine/classes/Elgg/Database/AccessCollections.php
View file
Open in desktop
Expand Up @@ -236,133 +236,6 @@ public function getAccessArray($user_guid = 0, $flush = false) {
return $this->hooks->trigger('access:collections:read', 'user', $options, $access_array);
}

/**
* Returns the SQL where clause for enforcing read access to data.
*
* Note that if this code is executed in privileged mode it will return (1=1).
*
* Otherwise it returns a where clause to retrieve the data that a user has
* permission to read.
*
* Plugin authors can hook into the 'get_sql', 'access' plugin hook to modify,
* remove, or add to the where clauses. The plugin hook will pass an array with the current
* ors and ands to the function in the form:
* array(
* 'ors' => array(),
* 'ands' => array()
* )
*
* The results will be combined into an SQL where clause in the form:
* ((or1 OR or2 OR orN) AND (and1 AND and2 AND andN))
*
* @param array $options Array in format:
*
* table_alias => STR Optional table alias. This is based on the select and join clauses.
* Default is 'e'.
*
* user_guid => INT Optional GUID for the user that we are retrieving data for.
* Defaults to the logged in user if null.
* Passing 0 will build a query for a logged out user (even if there is a logged in user)
*
* use_enabled_clause => BOOL Optional. Should we append the enabled clause? The default
* is set by access_show_hidden_entities().
*
* access_column => STR Optional access column name. Default is 'access_id'.
*
* owner_guid_column => STR Optional owner_guid column. Default is 'owner_guid'.
*
* guid_column => STR Optional guid_column. Default is 'guid'.
*
* @return string
* @access private
*/
public function getWhereSql(array $options = []) {

$defaults = [
'table_alias' => 'e',
'user_guid' => $this->session->getLoggedInUserGuid(),
'use_enabled_clause' => !access_get_show_hidden_status(),
'access_column' => 'access_id',
'owner_guid_column' => 'owner_guid',
'guid_column' => 'guid',
];

foreach ($options as $key => $value) {
if (is_null($value)) {
// remove null values so we don't loose defaults in array_merge
unset($options[$key]);
}
}

$options = array_merge($defaults, $options);

// just in case someone passes a . at the end
$options['table_alias'] = rtrim($options['table_alias'], '.');

foreach (['table_alias', 'access_column', 'owner_guid_column', 'guid_column'] as $key) {
$options[$key] = sanitize_string($options[$key]);
}
$options['user_guid'] = sanitize_int($options['user_guid'], false);

// only add dot if we have an alias or table name
$table_alias = $options['table_alias'] ? $options['table_alias'] . '.' : '';

if (!isset($options['ignore_access'])) {
$options['ignore_access'] = $this->capabilities->canBypassPermissionsCheck($options['user_guid']);
}

$clauses = [
'ors' => [],
'ands' => []
];

$prefix = $this->db->prefix;

if ($options['ignore_access']) {
$clauses['ors']['ignore_access'] = '1 = 1';
} else if ($options['user_guid']) {
// include content of user's friends
$clauses['ors']['friends_access'] = "$table_alias{$options['access_column']} = " . ACCESS_FRIENDS . "
AND $table_alias{$options['owner_guid_column']} IN (
SELECT guid_one FROM {$prefix}entity_relationships
WHERE relationship = 'friend' AND guid_two = {$options['user_guid']}
)";

// include user's content
$clauses['ors']['owner_access'] = "$table_alias{$options['owner_guid_column']} = {$options['user_guid']}";
}

// include standard accesses (public, logged in, access collections)
if (!$options['ignore_access']) {
$access_list = $this->getAccessList($options['user_guid']);
$clauses['ors']['acl_access'] = "$table_alias{$options['access_column']} IN {$access_list}";
}

if ($options['use_enabled_clause']) {
$clauses['ands']['use_enabled'] = "{$table_alias}enabled = 'yes'";
}

$clauses = $this->hooks->trigger('get_sql', 'access', $options, $clauses);

$clauses_str = '';
if (is_array($clauses['ors']) && $clauses['ors']) {
$clauses_str = '(' . implode(' OR ', $clauses['ors']) . ')';
}

if (is_array($clauses['ands']) && $clauses['ands']) {
if ($clauses_str) {
$clauses_str .= ' AND ';
}
$clauses_str .= '(' . implode(' AND ', $clauses['ands']) . ')';
}

if (empty($clauses_str)) {
$clauses_str = '1 = 1';
}

return "($clauses_str)";
}

/**
* Can a user access an entity.
*
Expand Down