Revert "refactor(RichObjectStrings): Only log error if key or value i…

[nickvergessen]

…s not string in validator"

• This reverts commit fd156d3.
• This reverts refactor(RichObjectStrings): Only log error if key or value is not string in validator #52035
• For reasoning see Revert "refactor(RichObjectStrings): Only log error if key or value i… #52059 and refactor(RichObjectStrings): Only log error if key or value is not string in validator #52035 (comment)
• @provokateurin Assuming the idea was to reduce the log messages, this PR failed it's purpose
• Also the spaming was quite low and therefore I'm not sure what the goal was
• If the idea was to alert developers more, we need to wrap the additional logging with a check for debug. But removing the throwing was even a change to the API and method calls that were expected to throw did not anymore.

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)

[provokateurin]

But throwing the exception is breaking a lot of things as well. Can you just downgrade it to debug, so it no longer spams on production instances?

[nickvergessen]

But throwing the exception is breaking a lot of things as well.

What is breaking, it is documented like that and 8 of 9 places calling the validation function catch the exception and change their behaviour based on it.

[susnux]

What is breaking, it is documented like that and 8 of 9 places calling the validation function catch the exception and change their behaviour based on it.

👍 why should the correct behavior here be changed just be cause of some edge cases of faulty apps not following the documentation? Better to fix it instead in the apps than accepting the error (which again can lead to unexpected errors)..

[susnux]

Can we on line 59 add mire context to the exception?
E.g. log the subject? Otherwise finding the origin is quite hard.

[nickvergessen]

Notifications is handling this properly already, catching the exception and logging an app+subject message instead:

server/lib/private/Notification/Manager.php

Lines 366 to 369 in ed6d76e

	if (!$notification->isValidParsed()) {
	$this->logger->info('Notification was not parsed by any notifier [app: ' . $notification->getApp() . ', subject: ' . $notification->getSubject() . ']');
	thrownewIncompleteParsedNotificationException();
	}

[nickvergessen]

• Improved the handling in the activity app the same way fix: Improving logging of invalid parameters for activity subject and… activity#1975

[provokateurin]

👍 why should the correct behavior here be changed just be cause of some edge cases of faulty apps not following the documentation? Better to fix it instead in the apps than accepting the error (which again can lead to unexpected errors)..

The problem is mainly the activity app, because its entries are generated by other apps and it's near impossible to figure out where the source of the error is without checking literally every instance in the code. See #51427 for example.

[nickvergessen]

Okay that should be covered by nextcloud/activity#1975 now