Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
#1848264: Compare and merge PhpWiki diff*.php with MediaWiki's DairikiDiff.php and DiffEngine.php has been closed and there is an @todo for it that can be removed. However there does remain remnants of DiffEngine which still do not pass coding standards.
Steps to reproduce
Proposed resolution
1/ Remove this from phpcs.xml.dist:
<!-- @todo This rule may be removed when https://www.drupal.org/node/1848264 is resolved. -->
Remaining tasks
- Merge request
User interface changes
API changes
DiffFormatter.php has three public properties which change.
show_header -> $showHeader
leading_context_lines -> $leadingContextLines
trailing_context_lines -> $trailingContextLines
line_stats -> $lineStats
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|
Issue fork drupal-3346401
Show commands
Start 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
Comments
Comment #3
dineshkumarbollu commentedHi
I am working on it.
Comment #5
dineshkumarbollu commentedComment #6
spokje- I think the test-failures are unrelated, random JavaScript failures.
- Left a comment in the MR.
Comment #9
vbouchetPlease find a patch for 11.x branch.
The @todo was about removing a rule once an issue (https://www.drupal.org/node/1848264) was fixed so I removed the rule and the @todo.
Comment #10
smustgrave commentedCC Failure.
Comment #11
vbouchetI did not realised that removing the exclude line would make phpcs to execute on more files. Please find an updated patch which fixes all the violations that was listed (I hope ;-)).
Comment #12
smustgrave commentedLooks good
Comment #13
quietone commented@vbouchet, That you for your interest in this issue. There was no need to make a patch because there is an existing MR with a fix. When an issue has an existing patch or MR it is best to continue working on that and not switch. More often than not it causes confusion.
The patch in #11 is perhaps for another issue? There is an MR and patch here, what has been RTBC'ed.
@smustgrave, I appreciate all the work you do to keep issues moving along! However, remember when marking an issue RTBC to explain what you did to review and research the change. Also, according to Examples of what will usually not receive credit credit is not given for Review or RTBC with only comments like "Looks good."
This needs a clear statement letting the committer know what has been RTBC'ed and why. Setting to needs work.
Comment #14
vbouchetThanks for the feedback @quietone. The MR was created on 10.x branch, needed rebase and there was a comment mentioning that the MR was not ready to be merged, this is why I went the shortest route for me.
I confirm the patch in #11 is for this issue and this is what has been reviewed and RTBC'ed.
What the patch does:
- Removes the @todo as per the issue description.
- The @todo were mentioning to remove the line after once an issue was fixed. This is the case so I removed the line which were excluding a directory from phpcs analysis.
- Fix the phpcs violation in the directory which was previously excluded.
Keeping as "Needs work" as I don't know what is the next step now.
Comment #15
quietone commented@vbouchet, I was mistaken. I just didn't read this change correctly. Sorry about the confusion and thanks for the query.
But there is still a problem because this is changing third party code something we should not do. I think this should be postponed until Drupal 11 when the DiffEngine is removed from core. I will check with the other committers. I am setting PMNMI. And assigning to myself as a reminder.
Regarding rebasing there are instructions on Drupal.org for rebasing to a new branch or the same branch.
Comment #16
quietone commentedSo, I have confirmed that we don't change the third party code (it would make updating it harder). The code is question though is being deprecated. So, let's remove the @todo in Drupal 11, after DiffEngine is removed.
I've noted this on #3295574: [meta] Remove deprecated classes, methods, procedural functions and code paths outside of deprecated modules on the Drupal 11 branch
Comment #17
quietone commentedThe issue in the @todo is now closed #1848264: Compare and merge PhpWiki diff*.php with MediaWiki's DairikiDiff.php and DiffEngine.php and the deprecated code has been removed, #3424436: [11.x] Remove deprecated code from the Diff component.
Setting this back to Needs Work.
Comment #18
ronttizz commentedI will be working on this for today. Assigning to myself.
Comment #19
ronttizz commentedI am a bit confused here. There is couple of branches but I seem to have hard time setting these up and testing them.
Comment #20
ronttizz commentedComment #21
ronttizz commentedI removed the suggested line.
Comment #22
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 #23
mradcliffeI performed Novice Triage on this issue. I am leaving the Novice tag on this issue because I think we can update the issue summary to include the helpful comments by @quietone and update the issue fork to Drupal 11.
Comment #24
xjmPlease also be sure to hide the non-canonical merge requests. Thanks!
Comment #25
phaedrus commentedI'm looking at this issue during the first time contributor mentored session at Portland2024
Comment #29
simeThe comment at #17 suggests that DiffEngine has been removed from core but there is still a lot of code at core/lib/Drupal/Component/Diff which appears to be remnants of DiffEngine. I see only deprecated code was removed in https://git.drupalcode.org/project/drupal/-/commit/b2477b3 which still left a lot of code.
So when @phaedrus (we are in contib sprint together) removed this exclude from the phpcs.xml.dist on d11, it has revealed a bunch of phpcs standards issues. I wonder will there be other issues that remove more DiffEngine code, so that this issue should remain blocked?
Comment #30
phaedrus commentedComment #31
phaedrus commentedComment #32
phaedrus commentedWe ended up touching several places in order to come up to current coding standards. As of this time, tests are passing.
Comment #33
andypostIt looks ready to go except renaming of public properties probably needs change record
Comment #34
smustgrave commentedWont' changing the public properties require the BC dance? As this could be a breaking change right?
Comment #35
simeComment #36
simeComment #37
simeSo, ideally the three public properties of DiffFormatter.php should be deprecated first? We could tell phpcs to (inline) ignore these three lines for now and mark them deprecated.
Comment #38
simeComment #39
andypost+1 to deprecate (not sure there's BC except magic get/set) for removal in 11.x
Comment #40
smustgrave commentedYea we will have to do the BC tango it seems. As @andypost mentioned _get/_set should be the route.
He shared this example via slack https://git.drupalcode.org/project/drupal/-/blob/10.3.x/core/lib/Drupal/...
Comment #41
simeOh wonderful. So will it be "deprecated in drupal:10.3.0 and is removed from drupal:11.0.0"?
Comment #42
smustgrave commentedBelieve it depends if we can get ready before a release of 10.3 happens. Could be close but may be removed in 12 since an 11 tag for beta has already happened.
Comment #44
andypostAdded branch with deprecation for 10.3 and filed CR https://www.drupal.org/node/3446709
Also fixed remaining CS, except
phpcs:disable Drupal.Commenting.DocComment.ShortSingleLineasDiffFormatter.phpis deprecatedComment #45
smustgrave commentedDeprecation looks great! Lets see if we can get this in for 10.3
Comment #46
alexpottThis change is making public API changes - yes those variables are part of the public API as we can see in core's usage - for coding standard reasons. Given this is not really Drupal's code I'm not 100% convinced about the value of making this change - and it's too late for the 10.3.x ship. I think we should deprecate in 11.x and remove in 12.x if we want to make this change. I guess changing to typed properties is decent API ensuring bonues.
Comment #47
quietone commentedI have read the issue summary and comments. There is nice work here on keeping the issue summary update as well as figuring out how to do the deprecation. All of which is much appreciated!
What I can't find is a reason for changing the third party code. Looking into the history here, the original version of this issue summary questioned fixing the coding standards by using a simple statement, "Fix Coding Standards?". In #16 I stated that I confirmed that we don't change third party code, and this point is again confirmed alexpott in #46. Changing the code for coding standards was then questioned in #29 but then there is no discussion on that topic before it was added to the issue summary in #35. It is unfortunate that I didn't update the issue summary at the time on #16.
So, what to do here? We should remove the @todo but only that line. And keep the line that excludes the directory from coding standard checks.
And I did some research. The @todo was introduced in #2887052-22: Ignore Diff component files in phpcs coding standards. It seems there was a misunderstanding about what files were going to be to changed,
Comment #48
quietone commentedSo, while I was writing this I was interrupted by a call that was demanding. When I came back I missed alexpott's comment about changing to typed properties. That's a good point to consider. I'll ask the other committers.
Comment #49
simeI agree, when I dived in it seemed like the scope of previous issues had shifted.
We were surprised when the issue which was claimed to remove a whole library only removed part of it. We noticed there were only a few coding standards to fix so we went with that.
"We" is me and Phaedrus in contrib room.
Comment #50
damienmckennaIt's the end of July, 11.0.0-rc1 is out already, what's the plan here?
Comment #51
andypostLooks it can go to 10.4.x but diff library still a question for me in terms of BC
Comment #52
andypostNeeds rebase and deprecation for 12.x
Comment #53
xjmOK, this issue is long and meandering and seems to have changed scope, so forgive me if I misunderstand.
The D11 MR appears to be making changes to a component we explicitly do not change. It was more than just a PHPCS rule; it was that we actually wanted to keep the code in line with upstream. Are we giving up forever on that? If so, I'd like to see an explicit framework manager signoff, and a justification in the IS that's not just about followups and PHPCS cleanups.
Also, adding typehints to public properties is a BC break.