Knowing which normalizer is going to be used without an instance of data in hand is essential to providing a reliable schema.
By adding the concept of a "deterministic" normalizer, we can start opting in normalizers that are capable of providing a reliable schema.
See #3022583: [META] Normalization System: clean up/speed up/provide schema for additional context.
Proposed Solution
Introduce a DeterministicNormalizerInterface with a getSupportedInterfaceOrClass() method.
If this interface is implemented, it signifies that the normalizer is valid for any instance of a particular interface or class and the given format. It should not be invalid for any particular value or $context.
Outcome
This will bring us a step closer to full schema support.
Additionally, we can avoid thousands of unnecessary calls to supportsNormalization in many instances.
Rough idea attached.
| Comment | File | Size | Author |
|---|---|---|---|
| #3 | 3031214-2.patch | 4.68 KB | gabesullice |
| #3 | interdiff.txt | 2.4 KB | gabesullice |
| deterministic-normalizers.patch | 3.71 KB | gabesullice |
Comments
Comment #2
gabesulliceComment #3
gabesulliceActually, I think we'll need this method if we want it to be useful for schema. The first patch works fine for a normalizer cache, but it won't work for generating a schema based on a class type.
Comment #4
gabesulliceComment #5
wim leersSo … this does not need review yet, right?
Comment #6
gabesulliceI expected the tests to fail 😅
I didn't want to bother you until they were passing.
Comment #7
wim leersThis looks a lot like https://github.com/symfony/symfony/pull/27049 — Symfony 4.1's
CacheableSupportsMethodInterface. But we won't get Symfony 4 until Drupal 9, so we can't use it. Tough.(They subsequently had to refine it in https://github.com/symfony/symfony/pull/27105.)
OTOH, what's proposed here is stricter — PR 27049 only communicates intent or promise, the proposal here guarantees it.
Thoughts?
Comment #8
gabesulliceFirst PR:
Whoa, it's surprisingly similar! I'd never seen that before.
Biggest difference: It's more like my first iteration (before the introduction of the
supportsNormalizationForInterfaceOrClassmethod). This approach doesn't let you figure out which normalizer will be used by$type, an important step for schema support. Right now, I'm just using it for theassert(I think this is the "strictness" you mean?), but it will play an important role later.I thought I saw a cool feature in their approach, but then realized I imagined it. It was a good idea nonetheless, and now I can claim it as my own!
We should do more than cache the normalizers that do apply... We can and should also keep a record of the ones that don't apply. IOW, if a normalizer is deterministic and is FALSE for a type, we should not ever reevaluate it again for the given type (this will happen if the the normalizer that does apply is not deterministic).
Second PR:
Hmmm. So, I think what this means suggest is that if core were to adopt this interface (which I hope it someday will), then we couldn't implement it on
NormalizerBasesince we cannot be certain that normalizers extendingNormalizerBaseare actually deterministic. 👎This PR gets around that by letting the extending classes return true or false WRT whether they are deterministic or not. I don't like this. Instead, I think it would be better to instead let every extending classes opt-in by implementing the interface downstream (we can put the method on the base class w/o implementing the interface to make it incredibly simple). In Drupal 9, we can make the interface required and then add it to the base class.
Which leads me to this q: should we make this a sister issue for this in core?
Comment #9
wim leersAgreed. We want to go further than that upstream addition.
Me neither.
For this patch or for the Symfony PR?
Comment #10
gabesulliceThis patch.
Comment #11
wim leersRelated: #2575761: Discuss a better system for discovering and selecting normalizers.
Comment #12
gabesulliceMoving to Drupal core
Comment #13
geek-merlinWow!
I was first confused about the naming "deterministic" ("what depends on what"). Found a nice issue how symfony struggled with naming, which imho is worth reading: https://github.com/symfony/symfony/pull/27210
I like something like "cacheableSupportsNormalization" (just my 5 cent, no strong feelings).
Yes, naming is hard...
Comment #14
xjmThis would be a minor-only addition. Since 8.9.x and 9.0.x are now in beta, I'm moving this to 9.1.x. Thanks!
Comment #17
gabesulliceWe need tests to show that this works as expected. Then, we should update the IS to summarize the purpose of the change and the proposed solution in the patch. Finally, this will need a change record and eventually a release note.
Comment #19
gabesulliceDrupal 8 is EOL and Drupal 9 is out. That means we can use the Symfony interface today. We'll lose the performance improvement of caching normalizers that don't apply (mentioned in #9), but it's probably better to do that upstream in Symfony now.
Issue for the Symfony interface here: #3252872: Use CacheableSupportsMethodInterface for performance improvement in normalizers
Comment #20
wim leersPer #19.