\Drupal\media\MediaSourceFieldConstraintsInterface::getSourceFieldConstraints says the return type should be
* @return \Symfony\Component\Validator\Constraint[]
* An array of validation constraint definitions, keyed by constraint name.
* Each constraint definition can be used for instantiating
* \Symfony\Component\Validator\Constraint objects.
But actually, its an array where the keys are validation plugin IDs and the values are options for the plugin.
Update the documentation
| Comment | File | Size | Author |
|---|
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !5594changes, plain diff MR !4264
Comments
Comment #2
anchal_gupta commentedComment #3
anchal_gupta commentedI have uploaded the patch. Please review it.
Comment #4
anchal_gupta commentedComment #5
smustgrave commentedSeems the 2nd part was left out.
Comment #6
nitin_lamaComment #7
nitin_lamaI didn't get the second part. Can you explain @smustgrave what exactly needs to be done here?
Thanks.
Comment #8
smustgrave commentedDoesn't mention what the values are.
Comment #11
roshni27 commentedAs mentioned in point #5, I have updated .
Please review.
Comment #12
smustgrave commented@roshnichordiya issue was moved to a MR that's on the correct branch. So work should continue there please. Hiding #11
Comment #13
elberComment #14
smustgrave commentedIS talks about updating issue summary. But MR is adding typehints to the functions.
Comment #16
marvil07 commented@smustgrave,
Yes, the current MR is doing changes outside the scope of the IS, namely it is introducing type hints on several places.
Considering the scope here was about to fix documentation, doing only that change is likely what is wanted here.
Even if MR seems to be preferred in general, the existing patch at #11 is already providing the needed change here; and I verified it stills apply correctly.
Hence, I am adding it back for consideration.
The other route is to change the MR to almost rollback everything, or opening a new branch, but that may be too much for the actual 2-lines documentation change here.
Comment #17
smustgrave commentedAgree patch #11 seems more in line.
Also FYI for a lot of people here, issue was tagged novice for new users. @elber you have over 2000 posts and @roshni27 almost a 1000. Think you are experienced enough and can handle the non novice issues. :)
Comment #18
xjmUnless these APIs were only added in 10.2.x, adding new typehints to existing code is a breaking change. We have another RTBC I've already tagged to that effect. They really need to be attached to a parent meta (which probably already exists) where we can discuss it.
Comment #19
dpi@xjm the most recent MR is off the rails. The doc only changes are recommended at this time, per previous comments. Can you provide new analysis/remove tag?
Comment #20
longwaveI agree the typehints are out of scope here and it looks like this should be a docs-only fix. But #11 only changes
MediaSourceFieldConstraintsInterfacewhereas the MR also does change the docs inMediaSourceEntityConstraintsInterface. If this is correct then we should change both at once.I suggest opening a new MR rather than using patch workflow.
Comment #22
marvil07 commented@longwave, that makes sense.
I changed the issue title accordingly too.
I opened a new MR 4264 based on patch #11 and the similar change in the MR for other interface.
Marking the path on #11 as hidden, as well as the previous MR for easier review.
Please feel free to skip contribution credits for me here if needed, since this is a novice task.
I decided to go ahead since (i) there was no activity here for a couple of weeks, (ii) extracting the bits from both an MR and a patch sounds a bit less of the novice side, and (iii) improvements on DX are nice to have.
Comment #24
marvil07 commentedComment #25
smustgrave commentedChange seems fine.
Comment #26
xjmThanks @dpi for explaining the situation and @longwave for getting it all rescoped correctly.
I had a very small grammar nitpick with the final version, which I fixed on the MR.
Comment #30
xjmCommitted to 11.x, 10.2.x, and 10.1.x as a patch-safe documentation bugfix. Thanks!