After looking at prototyping code I think it's best to leverage the symfony validator for #1845546: Implement validation for the TypedData API and #1696648: [META] Untie content entity validation from form validation. bschussek was so nice to implement some changes we need for the validator being useful for us, which got merged recently - see https://github.com/symfony/symfony/pull/6096. Given that changes integrating the symfony validator seems to be a good fit for our needs.
In short the reasons for using the symfony validator instead of rolling our own are
- Avoid re-inventing the wheel, but collaborate obviously.
- Most needed constraints already exist and have tests, so we can reuse them. So alltogether this saves us quite some work.
- Sophisticated features that are solved for us, e.g. like validation groups or the ability to define constraints that apply to the first field item only, or to the first field items date column.
- New developers might already new the API and/or conepts of it.
Attached is a first patch which adds the latest symfony component to core. I've added it via composer. (For that to work I had to temporarlily remove the serializer component - looks like it was missing from composer.lock. Anyway, there are no serializer component changes in the end.)
Comment | File | Size | Author |
---|---|---|---|
#48 | d8_validation_symfony.patch | 703.15 KB | fago |
#47 | d8_validation_symfony.patch | 703.18 KB | fago |
#43 | d8_validation_symfony.patch | 666.19 KB | fago |
#38 | d8_validation_symfony.patch | 654.29 KB | fago |
#19 | d8_validation_symfony.patch | 691.53 KB | fago |
Comments
Comment #1
fagoNote: The patch adds in also translation resources for the violation messages what makes it big. However, we won't use them directly, so maybe we want to remove it?
Comment #2
sunIf it's possible to remove them via composer, then I think we should. If not (i.e., if that would be a manual operation), then not.
Comment #3
effulgentsia CreditAttribution: effulgentsia commentedIn https://groups.google.com/d/topic/symfony-devs/oNaXdV5UGRU/discussion, fabpot listed the Validator component in the least BC stable group. So was the Serializer component, but Dries accepted that. Assigning to Dries, since he'll need to make the call. Is there any updated info available on what is and isn't stable?
Comment #4
fagoYeah, I think following the same approach as for the Serializer component would work. I'll reach out to symfony folks for verifying its status - at least it's listed to have some stable API.
ad #2: I think it would be doable with adding in a composer post-install/update script - however I'm not sure doing so would be a composer best practice...
Comment #5
attiks CreditAttribution: attiks commentedPatch to solve #2, but will only work on *nix systems, we can do the same using PHP, see http://getcomposer.org/doc/articles/scripts.md
Comment #8
attiks CreditAttribution: attiks commentedThis should work better
Comment #9
attiks CreditAttribution: attiks commentedIgnore the patch in #5 it updated everything except adding validator
Comment #10
fagoThe current validation code over at #1845546: Implement validation for the TypedData API depends on this PR - so if it gets accepted we should make sure it's included.
Comment #11
attiks CreditAttribution: attiks commentedSince this is blocking and time is running short.
Only question: do we use 'rm -rf' or do we prefer a PHP implementation?
Comment #12
RobLoachAnother solution is to add it to core/vendor/.gitignore so that it's not added to the git repository. Benefit is that:
Built with
drush composer install --prefer-dist
.Comment #13
effulgentsia CreditAttribution: effulgentsia commentedWhat's causing the extra trailing slash in the namespace name? We haven't yet switched to Composer's autoloader, and instead this gets passed to Symfony's, and I don't know if the trailing slash is a problem there. In any case, it's inconsistent with the other entries.
Comment #14
attiks CreditAttribution: attiks commented#13 Assuming this is about the patch in #8, to be honest, no idea I ran
composer require symfony/validator
, all the magic was done by composerComment #15
RobLoach#13 That's an update from Symfony and is taken into consideration over at #1658720: Use ClassLoader instead of UniversalClassLoader.
Comment #16
attiks CreditAttribution: attiks commentedApproach in #12 works as well, assuming #13 isn't a blocker.
Comment #17
catch@fago - have you heard back about the stability of this yet?
Comment #18
attiks CreditAttribution: attiks commented#17 most of the interfaces are already tagged with @api, see https://github.com/symfony/Validator/blob/master/ConstraintViolationList...
Comment #19
fagoI've reached out to bschussek for that, but no answer yet.
It is a problem - the autoloader doesn't work with it. So it's a blocker ;) Re-rolled patch just with the namespace fixed (manually). Not sure why, but composer seems to generate it that way now.
Yep, good idea!
Comment #20
RobLoachIt's an update from Symfony. You can see in their composer.json, they declare
Symfony\\Component\\Validator\\
. This change is made so that the autoloader has an easier time finding classes. It's fixed by either:I vote for #1 if we get it working with the bootstrap.
Comment #21
effulgentsia CreditAttribution: effulgentsia commented#12 and #19 only differ by that one trailing slash. I'd be fine with either one going in. If #12 goes in, we can deal with fixing it / working around it as part of #1845546: Implement validation for the TypedData API if that issue becomes ready before we've accomplished #20. If #19 goes in, then we can undo that one manual change after fixing #20.
Comment #22
fagoAs said, #12 lets the autoloader fail with the component. So let's use #19 and fix the issue with either #20.1 or #20.2
Comment #23
attiks CreditAttribution: attiks commentedRTBC for #19, this will be the fastest way forward.
Comment #24
catchThere's a difference with this vs. the serializer.
With the serializer we've not been using it to replace any existing core functionality, only to add new functionality which is outside the critical path. So if it turned into a bc disaster or there were other issues then it'd mess up the JSON-ld work but not much else (there's an issue to convert RSS feeds but those don't count as critical path either).
Using this for entity validation puts it right into the middle of core so it's a much more fundamental dependency to be adding.
I haven't reviewed the component yet and need to get caught up on the implementation issue as well...
Comment #25
fagoad #24: I've been told that everything tagged with @api is stable - what applies to all symfony components. So as long as we stick to that we are fine - right now we don't, but that's the remaining point being worked on, see http://drupal.org/node/1845546#comment-6781950. I asked Bernhard to "officially" clarify that for us though.
@Bernhard: I assume interfaces are part of the stable API also, right?
Related issue: #1852106: Add the symfony translator interface or translation component
Comment #26
Dries CreditAttribution: Dries commentedI haven't had a lot of time to look into this. Based on limited research, I think this is a good idea in theory. However, I agree that we need to understand the practical implications (e.g. stability of API). In that sense, I agree with catch in #24.
I would love to have some level of commitment that allows us to manage the risk or otherwise have some automation to make sure we don't use unstable APIs. Maybe something that could be added to the testbot or so?
Comment #27
webmozart CreditAttribution: webmozart commentedSorry for the delayed response. I just returned from the Symfony conference in Berlin today.
I would like to clear the doubts about the stability commitment of Symfony. Basically, the rules are very simple:
So it really boils down to the @api tags: Currently, a large part of Symfony's API (= mainly interfaces and central classes) are already tagged as @api. These tags are put on code that has been proven to work.
The recent refactorings to the Validator are new and thus are not marked as @api yet. We will mark the Validator interfaces as @api with the release of 2.2, unless we (or Drupal) discover problems with this new API that cannot be solved until 2.2. Note that no code previously tagged as @api has changed in these refactorings.
We currently have a related Symfony issue that might be interesting for you:
https://github.com/symfony/symfony/issues/6129
Comment #28
Crell CreditAttribution: Crell commentedbschussek: So it sounds like we still have a window for a few more months where we could try out the Valdiator and push change requests back upstream if we need them, before it gets locked down for-reals in ~May, right?
That to me sounds like a "commit now so we can make sure it's fully baked for our use when it does freeze" situation.
Comment #29
catchThat sounds encouraging to me, thanks bschussek!
Comment #30
webmozart CreditAttribution: webmozart commentedExactly.
Comment #31
effulgentsia CreditAttribution: effulgentsia commentedSo if we're adding a component to Drupal that has some public methods annotated with @api and some not, how do we verify that patches submitted to Drupal core are only using the @api ones? Relying on reviewers/committers to manually do this is too fragile. Any way to automate it? I don't think we need to block commit of this on that automation, as long as we think such automation is reasonable to accomplish by D8 release.
Comment #32
catchWe could subclass it and throw exceptions from the non @api methods? Then remove the subclass later?
Comment #33
fagoThanks a lot for the clarification bschussek!
Indeed, that seems to be a really good fit. In particular it applies to Metadata* interfaces that bschussek recently introduced for us. As we now they will be tagged stable we can start using them now, so we are good when they are frozen in February.
Comment #34
fagoNote that the validator is potentially introducing a dependency on the Symfony translation component via this PR. That works well for us as we can easily implement the interface to plugin in our translation system - so the question remaining is how we handle the translation component dependency. See #1852106: Add the symfony translator interface or translation component
Comment #35
RobLoachThere are a few issues stalled by this, can we get it in soon, please? The approaching feature freeze scares me.
Comment #36
effulgentsia CreditAttribution: effulgentsia commented#19: d8_validation_symfony.patch queued for re-testing.
Comment #38
fagoGuzzle got added - I re-ran composer and updated the patch.
Comment #39
chx CreditAttribution: chx commentedPostponed on #1852106: Add the symfony translator interface or translation component
Comment #40
Dries CreditAttribution: Dries commentedI'm comfortable with this being committed after the translation issue is resolved.
Comment #41
Dries CreditAttribution: Dries commentedOh, I like catch's idea in #32 as well.
Comment #42
fagoUpdated the patch to work inline with recent composer.json adaptions.
Comment #43
fagoComment #44
fagoComment #45
attiks CreditAttribution: attiks commentedFYI: https://github.com/symfony/symfony/pull/6137 is merged!
Comment #46
effulgentsia CreditAttribution: effulgentsia commentedGreat! I'm assuming #1853096: Integrate symfony validation violations with Drupal translation is the next step towards resolving #39 and then unpostponing this issue?
Comment #47
fagoSymfony validator uses the TranslatorInterface of symfony translation to enable us translating violation messages. Attached patch integrates a solution for #1852106: Add the symfony translator interface or translation component - it again makes use of the .gitignore trick to only add the needed TranslatorInterface of symfony translation.
Patch attached.
Comment #48
fagoAdded missing new-line at the end of .gitignore.
Comment #49
fagoClarification: This patch adds the TranslatorInterface of Symfony/Translation as it is required by Symfony/Validator now, but not the rest of the Translation component. We will implement this interface with our own class to plug-in Drupal translations - Symfony translations won't be in any use at all.
Comment #51
fago#48: d8_validation_symfony.patch queued for re-testing.
Comment #52
effulgentsia CreditAttribution: effulgentsia commentedFantastic. Let's make sure we have a follow up for #32 / #41. Maybe it could/should be a combination of subclassing and .gitignore (the latter for e.g., the Mapping subnamespace)?
Comment #53
webchickLooks like Dries OKed this in #40, pending the "two translation systems" issue getting resolved, and according to #49 it sounds like it has.
However, no longer applies. :( Sorry, I saw #1894508: Update symfony/serializer first. Eclipse is working on a re-roll.
Comment #54
webchickOk, I lied. #1894508: Update symfony/serializer looks much easier to re-roll than this so, reverted that one instead.
Committed and pushed to 8.x. Thanks!