Page MenuHomePhabricator

Introduce StatusValue::ignore method
Open, LowPublic

Description

Sometimes, especially with permissions, we need to ignore a certain permission error. For example, in EditPage and perhaps in other places.

Proposed API of the method:

	/**
	 * Removes an error or warning.
	 *
	 * @param string|MessageSpecifier $message
	 */
	public function ignore( $message ) {
		// implementation goes here
	}

Tests should be added for the new feature.

Event Timeline

Change 656547 had a related patch set uploaded (by Ammarpad; owner: Ammarpad):
[mediawiki/core@master] Allow removing errors/warnings set in Status object

https://gerrit.wikimedia.org/r/656547

Relying on messages in this way seems rather fragile to me. Once an error is raised, I don't think we want to mutate it in backwards/looser ways downstream.

The EditPage issue seems a rather unusual exception to that, one that I think is a clear signal that it is doing something fragile that we should interpret as a nudge to find a better approach, not solidify it further by hiding the anti-pattern behind a built-in method.

As an example, the EditPage issue could be solved by providing the source of the error upfront with what is and isn't allowed, rather than tryng to guess how it will react and then ignoring some of its outcome and hope that there were no other side-effects.

The EditPage issue seems a rather unusual exception to that, one that I think is a clear signal that it is doing something fragile that we should interpret as a nudge to find a better approach, not solidify it further by hiding the anti-pattern behind a built-in method.

It's not entirely unusual, PermissionManager::getPermissionErrors which we are trying to replace, has an optional $ignoreErrors parameter, which is somewhat used. (should've been used in EditPage example as well) Not widely, but enough to qualify as not a fluke, so it's something we could support with a some interface.

the EditPage issue could be solved by providing the source of the error upfront with what is and isn't allowed, rather than tryng to guess how it will react and then ignoring some of its outcome and hope that there were no other side-effects.

The current replacement interface for getting full list of permission errors, defined in Authority patch is to pass an optional PermissionStatus (extends StatusValue) into permission check calls - if it's passed, all errors will be collected and added to the status. If not, we bail on the first error and just return a boolean. Alternative to ignoring the errors by mutating the status is to make yet another optional $ignoreErrors parameter. In that case EditPage and other callers still need to know specific error names to ignore - the difference would be just where the array_filter is applied. Seems like the same abstraction leak. Imho, the perfect for encapsulation we need to introduce new permissions (in EditPage example, it would be something like viewpreview and viewdiff), but that seems like an overkill.

TLDR: Both solutions (ignoring errors post-factum or passing ignored errors in) seems like a leaky abstraction. We can maybe move the ignore method down the hierarchy to PermissionError class?

cc @daniel

The EditPage issue seems a rather unusual exception to that, one that I think is a clear signal that it is doing something fragile that we should interpret as a nudge to find a better approach, not solidify it further by hiding the anti-pattern behind a built-in method.

Message keys sometimes double as error code. Status::hasMessage() is evidence of that, as is the logic for mapping from messages to API errors in ApiMessageTrait::getApiCode(). Especially for permission errors, we inspect the first element of the errors returned by getPermissionErrors() to see why a permission failed. I agree that it would be nice if we didn't do that, but changing this is going to be tough. I can see that adding a utility method that is based on this idea seems like we are making it easier to do the wrong thing. But since it's pretty much impossible to do the right thing, we'd only make people do the wrong in in harder and more messy ways by not having the utility functions...

Regarding hasMessage, this indeed indicates that message keys are sometimes used as part of the public "return" API in some legacy classes. However that's, I think, pretty far removed from making a Status object mutable to retroactively ignore something that was emitted and make it no longer ok/error/fatal.

When I suggested to declare "upfront" I was not referring to using the same fragile mechanism of message keys. While that is a small improvement (no longer mutatint Status but interacting directly with the source, telling it not to add certain errors to the Status), what I actually meant was to be explicit about the underlying logical meaning of those errors. E.g rather than saying status->ignore(cannot-upload) or something->doThing(ignore=cannot-upload), one would have it like something->doThing(upload: allowed).

Would that be difficult to add to the example at hand? (I don't know which example, but I'm sure there's one) If we don't have time/space to work on this, how much is the situation net-improved by adding to Status? Is it fine to just leave it as-is in that case until such time we can address it properly? Or perhaps using message keys upfront would be a reasonable compromise, where e.g. we still use message keys as stable API but rather than ignoring afterwards we pass the ones we want to ignore to the Status-issuer telling it not to use those. That way, we also avoid the issue of side-effects and long-term expectations matching up, it allows the issuer to know about it in advance and ensure other things will work as expected as well. It's nearly impossible to improve code if it is allowed to send errors that a consumer may then mutate/ignore, we don't have mechanisms in place to detect or deprecate that afaik.

one would have it like something->doThing(upload: allowed).

The interface I want to use the utility for is for Authority, which will be a generic interface for checking if some action is allowed or not. This it's a level lower than 'doSomething' and is expected to be used as a utility to implement 'upload: allowed'. Consider this code:

function doSomething( $options ) {
  $permissionStatus = new PermissionStatus();
  if ( !$this->authority->authorizeRead( 'my_fancy_action', $this->page, $status ) ) {
    if ( $options['allow_upload'] ) {
      $status->ignore( 'the_error_which_occurs_because_of_upload' );
    }
  }
  
  if ( $status->isOk() ) {
    $this->actuallyDoTheThing();
  }
}

So we still have a fragile mapping of message keys to their conceptual meaning encoded in code, but as Daniel says, changing the use of message keys as permission error reporting medium to something else is pretty much impossible. Thus, the mapping will need to happen somewhere, and StatusValue::ignore (like already existing StatusValue::replaceMessage) is a utility to do just that.

function doSomething( $options ) {
  $permissionStatus = new PermissionStatus();
  if ( !$this->authority->authorizeRead( 'my_fancy_action', $this->page, $status ) ) {
    if ( $options['allow_upload'] ) {
      $status->ignore( 'the_error_which_occurs_because_of_upload' );
    }
  }
  
  if ( $status->isOk() ) {
    $this->actuallyDoTheThing();
  }
}

So we still have a fragile mapping of message keys to their conceptual meaning encoded in code, […], the mapping will need to happen somewhere, and StatusValue::ignore (like already existing StatusValue::replaceMessage) is a utility to do just that.

has/replaceMessage don't turn failure into success.

In the above example, where does the_error_which_occurs_because_of_upload get added to the Status? Can't that code read $options and understand the intent upfront?

In the above example, where does the_error_which_occurs_because_of_upload get added to the Status?

the_error_which_occurs_because_of_upload is one of the possible errors that can happen while checking for my_fancy_action permission. It's added to StstusValue by Authority

Can't that code read $options and understand the intent upfront?

$options are dependent on a use-case context. Authority interface is generic interface to check if action is allowed. Action can be disallowed for a number of reasons. Sometimes, we don't care about some of the reasons.

Neither of the solutions here is a good one. Ideally, the need to ignore errors shouldn't really arise. I will do one more pass over the actual use-cases we have for this and see if there's a clearer way to solve it in every use-case. Some of them will be solved by making Status errors unique, will see what's left.