Remove @todo from phpcs.xml.dist linking to closed issue

- I think the test-failures are unrelated, random JavaScript failures.
- Left a comment in the MR.

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

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

CC Failure.

I 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 ;-)).

Looks good

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

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

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

So, 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 🌱 [11.x] [META] Remove deprecated classes, methods, procedural functions and code paths outside of deprecated modules on the Drupal 11 branch Postponed

The issue in the @todo is now closed 📌 Compare and merge PhpWiki diff*.php with MediaWiki's DairikiDiff.php and DiffEngine.php Closed: won't fix and the deprecated code has been removed, 📌 [11.x] Remove deprecated code from the Diff component Fixed .

Setting this back to Needs Work.

I will be working on this for today. Assigning to myself.

I am a bit confused here. There is couple of branches but I seem to have hard time setting these up and testing them.

I removed the suggested line.

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

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

Please also be sure to hide the non-canonical merge requests. Thanks!

I'm looking at this issue during the first time contributor mentored session at Portland2024

phaedrus → changed the visibility of the branch 3346401-remove-todo-from to hidden.

phaedrus → changed the visibility of the branch 3346401-remove-todo-comment to hidden.

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

We ended up touching several places in order to come up to current coding standards. As of this time, tests are passing.

It looks ready to go except renaming of public properties probably needs change record

Wont' changing the public properties require the BC dance? As this could be a breaking change right?

So, ideally the three public properties of DiffFormatter.php should be deprecated first?

+1 to deprecate (not sure there's BC except magic get/set) for removal in 11.x

Yea 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/...

Oh wonderful. So will it be "deprecated in drupal:10.3.0 and is removed from drupal:11.0.0"?

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

Added 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.ShortSingleLine as DiffFormatter.php is deprecated

Deprecation looks great! Lets see if we can get this in for 10.3

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

I 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,

So, 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.

I agree, when I dived in it seemed like the scope 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.

It's the end of July, 11.0.0-rc1 is out already, what's the plan here?

Looks it can go to 10.4.x but diff library still a question for me in terms of BC

Needs rebase and deprecation for 12.x