Lots of work has gone into Statement classes in the last couple of majors. Maybe we can now reflect that in the typehinting of StatementInterface methods' parameters.
Prepare new signatures for the methods in D11 according to https://www.drupal.org/about/core/policies/core-change-policies/how-to-d.... In D12, finalize the change.
| 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 !15426changes, plain diff MR !12031
Comments
Comment #3
mondrakeComment #4
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #5
mondrakeComment #7
smustgrave commentedFixed a minor cspell issue
Reviewing how other tickets like this have been believe this should be correct. Not comment!
Comment #8
mondrakeThanks for fixing that typo!
Comment #9
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #10
mondrakerebased
Comment #11
longwaveChange record links are invalid.
Also not very happy about the phpcs:ignore tags as they make the docs super hard to read - more meta tags than useful information! - but not sure what we can do about that.
Comment #12
mondrakeTotally missed the CR, sorry. Added a draft one.
#3531685: Ignore specifc sniffs when adding additional parameters to interface methods came up with a different format for the phpcs:ignore, @todo and @see dance. Attending the ball here.
Comment #13
mondrakeComment #14
dcam commentedI reviewed the most recent changes. The change record links are correct now. The new formatting of the tags seems very readable to me. I'm going to bump this back to RTBC.
Comment #16
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #17
mondrakebot fluke
Comment #18
mondrakeChanged inline @see references to a new meta, the previously referenced one was pointing to a closed D11 one. Leaving at RTBC.
Comment #19
godotislateA couple questions on the MR.
Comment #20
mondrakeNeeds some more work, on it.
Comment #21
longwaveWe only need to go through that preparation cycle when changing an existing typehint or adding a new argument I think?
Adding a new typehint to an existing argument on an interface should be fine: https://3v4l.org/Whe7r - this is the opposite way round to return types where we need to add them from the bottom up.
Comment #22
mondrakeThanks for #21, forgot about contravariance. Let's try
, we'd have to deprecate passing unexpected parameters on the base class.Comment #23
mondrakeComment #24
smustgrave commentedQuestion how come this one didn’t follow the same path as the other parameter tickets with it being commented out?
Comment #25
mondrake#24 because we are adding type to method parameters, and these are contravariant unlike return types that are covariant
From https://www.php.net/manual/en/language.o op5.variance.php:
It was like that till #21 when @longwave pointed out it was unnecessary.
Comment #26
smustgrave commentedOnly other question and don't mind marking, but should it be 13 vs 12?
Comment #27
mondrakeNo strong opinions
Comment #28
smustgrave commentedLets leave as is then and see if the core committer. I'm 50/50 since it's the database stuff but may be a none issue in the grand scheme of things. Thanks for indulging my questions!
Comment #30
amateescu commentedSearched contrib for any usage of
PDO::FETCH_ORI_*and found none, so I think removing in D12 is fine.Committed 645ffba and pushed to main. Thanks!
We'll need a 11.x backport MR.
Comment #33
mondrakeMR 15426 backports to 11.x
Comment #34
smustgrave commentedlooks like a good rebase
Comment #36
amateescu commentedCommitted ab14cc0 and pushed to 11.x. Thanks!