Skip to content

Conversation

@pounard
Copy link
Member

@pounard pounard commented Jul 24, 2025

In many occasions, we need to share context for a single anonyzation run between anonymizers:

All this data must go directly to anonymizers, but using the Options instance is wrong:

  • Even if we propagate some data into it, it is not shared (so goodbye sample table sharing).
  • Adding more and more options into it makes anonymizers unable to use those options names for themselves.

The less impactful solution I found is the following:

  1. Create a new Context class, which will carry all this information.
  2. Change constructor of AbstractAnonymizer in order to accept this new parameter.

But we have a problem here, it's BC break, and a huge one, it will cause a refactor of all our tests, and possibly create problems with existing custom anonymizers in the wild (I guess there's not much, but you never know). This can be mitigated by the fact that people should extend our abstract implementations and probably won't override the constructor, but you never know, it's public API now.

So, temporary ulgy solution:

  1. Make Context extend Options (even if it makes no sense) and pass it to AbstractAnonymizer::__construct(): no signature change needed.
  2. Adapt AbstractAnonymizer::__construct() to check validity of input, and create a dummy Context instance when Options are passed instead.
  3. Deprecate this signature and change it in 3.0 to pass two parameters: Options $options and Context $context (which will not extend Options anymore).

@pounard pounard force-pushed the anonymizator-context branch from be29e73 to eba1c0f Compare October 18, 2025 14:05
@pounard
Copy link
Member Author

pounard commented Oct 18, 2025

Following our discussions on matrix, I did remove the bc layer, rationale being:

  1. first of all, the AbstractAnonymizer constructor is final, no one is able to override it,
  2. anonymizers should never be instanciated using new, that's why the registry exists,
  3. Anonymizator is being instanciated using a factory as well, it should not be instanciated using new.

Impact for most if not all users should null.

Comment on lines +80 to 84
#[\Deprecated(message: "Will be removed in 3.0, use \$this->context->salt instead.", since: "2.1.0")]
protected function getSalt(): string
{
return $this->options->get('salt') ?? Anonymizator::generateRandomSalt();
return $this->context->salt;
}
Copy link
Member

Choose a reason for hiding this comment

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

I think we can directly get rid of this

Comment on lines -50 to 58
private AnonymizationConfig $anonymizationConfig,
private ?string $salt = null,
?Context $defaultContext = null,
) {
$this->logger = new NullLogger();
$this->output = new NullOutput();
$this->defaultContext = $defaultContext ?? new Context();
}

Copy link
Member

Choose a reason for hiding this comment

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

I in favor of keeping things the most simple as we can. Here, can we use the both options for the $defaultContext property ?

  1. If we always give a $defaultContext, I think we should not accept it to be null.
  2. If we never give a $defaultContext, then we should remove it from params and always prepopulate it in constructor.

I think we should not introduce behavior we don't need because it is more code to maintain, and it makes things more complicated to understand.

}

$plan = [];
$context = clone $this->defaultContext;
Copy link
Member

Choose a reason for hiding this comment

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

Given taht $context is readonly in AbstractAnonymizer, why do you need to clone it here ?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants