Skip to content

Conversation

@juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Feb 3, 2023

Summary

if ($this->activityManager->isFormattingFilteredObject()) {
try {
return $this->parseShortVersion($event);
} catch (\InvalidArgumentException $e) {
// Ignore and simply use the long version...
}
}
return $this->parseLongVersion($event);
could also get called on unrelated requests so we should make sure to not cause problems when trying to get params that don't exist on unrelated requests.

@nickvergessen Do we actually need that separation in the system tags provider to render them differently? My assumption is that this is then based on the activity API request, but feels like an odd place for checking the parameters there.

Fixes random errors like this:

LogicException: "put" can only be accessed once if not application/x-www-form-urlencoded or application/json.

Checklist

Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr requested review from a team, ArtificialOwl, blizzz, icewind1991 and nickvergessen and removed request for a team February 3, 2023 13:01
@juliusknorr juliusknorr added 3. to review Waiting for reviews bug labels Feb 3, 2023
@nickvergessen
Copy link
Member

Do we actually need that separation in the system tags provider to render them differently? My assumption is that this is then based on the activity API request, but feels like an odd place for checking the parameters there.

I think it happens like:

  • File is uploaded
  • Autotagging happens
  • Triggers an activity entry
  • Any user with the activity opted in for getting activities as push notifications
  • That means we render the activity
  • Parameters are checked

But also this is not a unique problem here. I wonder if IRequest should be save guarded instead?

@juliusknorr
Copy link
Member Author

I've also wondered if

throw new \LogicException(
should actually throw or just be a debug log. It could just return existing parameters from
$this->items['parameters'] = array_merge(

@juliusknorr
Copy link
Member Author

Pushed a generic approach to #36525

@juliusknorr juliusknorr closed this Feb 3, 2023
@skjnldsv skjnldsv deleted the fix/31478 branch March 14, 2024 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants