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

elgg_get_river() doesn't work as expected when using the 'view' attribute #10791

Closed
iionly opened this issue Feb 26, 2017 · 5 comments
Closed

Comments

@iionly
Copy link
Contributor

iionly commented Feb 26, 2017

Due to deprecation of elgg_delete_river() I've switched to the recommended new way ("Use elgg_get_river() and call delete() on the returned item(s)") in several plugins. Mistake of me: I've not checked the parameters of elgg_get_river() and just changed the code to use an ElggBatch with elgg_get_river() fetching the river items with basically the same parameters as used before in elgg_delete_river(). And it seemed to work.

BUT!

It seems using the 'view' attribute doesn't return exactly the river items I want to have but MORE.

Example:

$view = "river/user/default/profileupdate";

$river_items = new ElggBatch('elgg_get_river', array(
	'view' => $view,
	'action_type' => 'update',
	'subject_guid' => $user->guid,
	'limit' => false,
));

did not only the river items with the view river/user/default/profileupdate" but ADDITIONALLY items with views "river/user/default/profileiconupdate" and "river/object/poll/update" that I surely do NOT want to delete here.

It seems to work with a custom where clause:

$view = "river/user/default/profileupdate";

$river_items = new ElggBatch('elgg_get_river', array(
	'action_type' => 'update',
	'subject_guid' => $user->guid,
	'limit' => false,
	'wheres' => array("rv.view = \"$view\""),
));

But that's not really what you think of using in the first place.

So, bug or what?

And btw, there's a public usable replacement of elgg_delete_river() necessary urgently both for Elgg 2.3 and 3.0. It's just annoying that you either get the deprecated warning all the time (2.3 will get LTS so will be around for a while), or use the private _elgg_delete_river() or need to use much too much custom code to replace elgg_delete_river() by other means.

@mrclay
Copy link
Member

mrclay commented Feb 27, 2017

Ouch, views should've been added to elgg_get_river() before we deprecated that. A few options:

  1. If view(s) is passed as an option to elgg_get_river(), issue a warning that this feature is not yet implemented, and add a docs page showing how to uses wheres. (my preference)
  2. Un-deprecate elgg_delete_river(). Authors will have to re-code for 3.0.
  3. Add support as a bugfix in 2.3.2.

@mrclay
Copy link
Member

mrclay commented Feb 27, 2017

Actually, adding the feature is super trivial. (see #10792)

@hypeJunction
Copy link
Contributor

I see us getting rid of view column eventually and using hooks instead, so let's just not add any API that would be building upon it. Option 1 sounds good to me.

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Feb 27, 2017
mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Feb 27, 2017
Devs who refactor `elgg_delete_river()` code to use `elgg_get_river()`
may be surprised that it doesn't support this option.

Fixes Elgg#10791
@mrclay
Copy link
Member

mrclay commented Feb 27, 2017

PR #10793

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Feb 27, 2017
Devs who refactor `elgg_delete_river()` code to use `elgg_get_river()`
may be surprised that it doesn't support this option.

Fixes Elgg#10791
mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Feb 27, 2017
Devs who refactor `elgg_delete_river()` code to use `elgg_get_river()`
may be surprised that it doesn't support this option.

Fixes Elgg#10791
@iionly
Copy link
Contributor Author

iionly commented Mar 4, 2017

Fixed by #10791.

@iionly iionly closed this as completed Mar 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants