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

Implements #4453: entities/river queries alterable by hooks (WIP) #6256

Closed
wants to merge 1 commit into from

Conversation

mrclay
Copy link
Member

@mrclay mrclay commented Dec 4, 2013

DO NOT MERGE

TODO:

  • Allow handlers to see/change the getter function (e.g. to add a relationship constraint to an ege() call.)
  • Rebase
  • Gather more consensus around this.

After this was put in place we could add names to the prominent public-facing lists/river queries. I'd suggest names prefixed with "elgg:" or "core:". E.g. "elgg:activity/all", "elgg:activity/owner", "elgg:blog/all", etc.

With this in place, one could alter $options arrays simply:

// set the main activity stream limit to 50
function my_plugin_alter_river($h, $t, $v, $p) {
    $v['limit'] => 50;
    return $v;
}
elgg_register_plugin_hook_handler('river:options', 'elgg:activity/all', 'my_plugin_alter_river');

Or you could make reusable queries by putting a full set of options in the hook:

function my_plugin_fetch_foos($h, $t, $v, $p) {
    return array_merge($v, array(
        'type' => 'object',
        'subtype' => 'foo',
    ));
}
elgg_register_plugin_hook_handler('entities:options', 'my_plugin:foos', 'my_plugin_fetch_foos');

echo elgg_list_entities(array('query_name' => 'my_plugin:foos'));

'subject_guid' => $entity->guid,
'object_guid' => $entity->guid,
);
$id = elgg_create_river_item($params);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd love it if we checked if general get river actually finds this one first

Copy link
Member Author

Choose a reason for hiding this comment

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

Latest commit should make this clearer

@juho-jaakkola
Copy link
Member

We must create some kind of a guideline for naming the queries. There's not much use for the hooks if the names given by plugin devs aren't context-aware and unique enough.

@mrclay
Copy link
Member Author

mrclay commented Jan 14, 2014

I think these patterns would cover most of the built-in queries we'd want to make modifiable

core:(activity|blog|bookmark|file|pages)/(all|owner|friends)
core:groups/(all|member|owner)
core:group_discussion
core:group_members
core:members/(all|popular|online)
core:friends(of)?
core:messages/(inbox|sent)
core:group_module/(blog|bookmark|file|pages|activity|discussion)

@ewinslow ewinslow added this to the Discussion milestone Feb 6, 2014
@juho-jaakkola
Copy link
Member

Maybe we should remove core: from the plugins in case we some day end up placing bundled plugins to their own separate repos.

@ewinslow
Copy link
Contributor

ewinslow commented Feb 9, 2014

+1

On Sunday, February 9, 2014, Juho Jaakkola notifications@github.com wrote:

Maybe we should remove core: from the plugins in case we some day end up
placing bundled plugins to their own separate repos.


Reply to this email directly or view it on GitHubhttps://github.com//pull/6256#issuecomment-34587070
.

Evan Winslow
http://community.elgg.org/profile/ewinslow

@mrclay
Copy link
Member Author

mrclay commented Feb 17, 2014

  • Add RST docs
  • Add query names listed above (without core:)

This adds support for the option "query_name" in elgg_get_entities,
elgg_get_river, and elgg_get_annotations. The option causes the $options
array to be filtered by a plugin hook before the function executes the
query. Most core listings are also given query names, allowing devs to
alter built-in queries without overriding core systems.

Refs: Elgg#4453
@mrclay
Copy link
Member Author

mrclay commented Jul 12, 2014

Updated to simplify unit tests, improve phpdocs, add RST docs, and to give names to a ton of core listings, including the custom index, group modules, and some sidebar listings.

One thing we probably need to clarify here is that the $options used in core pages are subject to change, and these changes may break complex alterations via hook. In essence these currently internal queries sort of become API surface. I think, though, the users of these APIs will typically be more savvy devs.

Anyway, this adds more named queries than I was projecting earlier but the RST docs should document them all.

@mrclay
Copy link
Member Author

mrclay commented Jul 12, 2014

BTW the query_name APIs are tested, but I had to test the page listings manually to verify that the appropriate hooks were called with correct names.

@mrclay mrclay removed this from the Elgg 1.10.0 milestone Oct 20, 2014
@mrclay
Copy link
Member Author

mrclay commented Oct 20, 2014

Letting this sit for long term discussion. For 1.x a simple way to influence default limits would be great.

@Srokap Srokap added this to the Elgg 2.0.0 milestone Dec 1, 2014
@juho-jaakkola
Copy link
Member

Could it be possible to have 'all:options', 'all'? It could be used e.g. to set the same 'limit' value to all lists.

@juho-jaakkola
Copy link
Member

Just writing down an idea: Would it be possible to name the queries automatically based on the location of the file calling ele*()?

@mrclay
Copy link
Member Author

mrclay commented Dec 4, 2014

Names based on file would be chaotic and keep us from refactoring. Also several cases have multiple queries in same file.

@ewinslow
Copy link
Contributor

I think this discussion is highly related:

https://community.elgg.org/discussion/view/2158924/could-this-be-the-future-of-elgg-data-access-and-ui-rendering

The query API requires a router behind the scenes, which is a nice way to get pluggability without feeling hacky.

@ewinslow
Copy link
Contributor

I might be able to get behind naming the queries based on what they would be if they were written in a falcore-style router.

@ewinslow
Copy link
Contributor

Then we can check this in and any transition later would be relatively smooth...

@mrclay mrclay modified the milestones: Elgg 2.0.x, Elgg 3.0.x Jul 8, 2015
@mrclay
Copy link
Member Author

mrclay commented Aug 26, 2016

Willing to call this dead. I think this is too low-level and entangles user code too much with internal logic. I have a better idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants