Docs on return type on two media constraints are misleading

I have uploaded the patch. Please review it.

Seems the 2nd part was left out.

I didn't get the second part. Can you explain @smustgrave what exactly needs to be done here?
Thanks.

Doesn't mention what the values are.

As mentioned in point #5, I have updated .
Please review.

@roshnichordiya issue was moved to a MR that's on the correct branch. So work should continue there please. Hiding #11

IS talks about updating issue summary. But MR is adding typehints to the functions.

Rishabh Vishwakarma → made their first commit to this issue’s fork.

IS talks about updating issue summary. But MR is adding typehints to the functions.

@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 🐛 Return type on \Drupal\media\MediaSourceFieldConstraintsInterface::getSourceFieldConstraints is wrong Needs work 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.

Agree 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.

Unless 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.

@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?

I agree the typehints are out of scope here and it looks like this should be a docs-only fix. But #11 only changes MediaSourceFieldConstraintsInterface whereas the MR also does change the docs in MediaSourceEntityConstraintsInterface. If this is correct then we should change both at once.

I suggest opening a new MR rather than using patch workflow.

I agree the typehints are out of scope here and it looks like this should be a docs-only fix. But #11 only changes MediaSourceFieldConstraintsInterface whereas the MR also does change the docs in MediaSourceEntityConstraintsInterface. If this is correct then we should change both at once.

@longwave, that makes sense.
I changed the issue title accordingly too.

I suggest opening a new MR rather than using patch workflow.

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.

marvil07 → changed the visibility of the branch 3367151-return-type-on to hidden.

Change seems fine.

Thanks @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.

Committed to 11.x, 10.2.x, and 10.1.x as a patch-safe documentation bugfix. Thanks!

xjm → closed merge request !4264

xjm → closed merge request !5594

Automatically closed - issue fixed for 2 weeks with no activity.