Deprecate HandlerBase defineExtraOptions

I try to resolve issue please review.
Thanks!!

CR and Deprecation test coverage is missing, also I don't think the changes done in the MR is valid.

cc: @smustgrave

Per #5, this is indeed not the change we are looking for, we need to properly deprecate this in the base handler, we can't just remove it

Hi, as per the feedback in #5 and #6, I have:

1. Deprecated the defineExtraOptions() method in the HandlerBase class.
2. Added test to ensure proper deprecation handling.
3. Created a change record → for the deprecation.

Kindly review the changes and please let me know if any further modifications are required.
Thanks!

LGTM

The deprecated version needs to updated to 11.2.0 instead of 11.0.0.

Updated the depricated version.
I tried to solve the failing phpunit test but couldn't resolve it. Can anyone suggest something to pass that test?

@chhavi.sharma I have reviewed the pipeline failure : https://git.drupalcode.org/issue/drupal-3485084/-/jobs/3674676
and it seems like a random failure not relevant with the changes in MR, try to re-run it will fix. hope that help.

Comment on MR.

Maybe we can skip deprecation test since defineOptions isn't used anywhere. But the current test I don't believe is actually testing anything, just throwing a deprecation but no assertion.

Address the feedback on MR also rebase and fixed the conflicts, Moving this NR.Please review.

Change appears fine to me.

Added in deprecation notice that the method has no replacement. I assume it does not. But if it does have a replacement we need to state what it is.

Pipeline is green.

@smustgrave Can you please review the last commits? It seems these changes might not be necessary. I noticed that @oily has made similar changes in another ticket, which also seem unnecessary.

@oily, if you believe this change is required, please add a comment. I’m already working on this ticket, I will address.

@shalini_jha RE #22

I noticed that @oily has made similar changes in another ticket, which also seem unnecessary.

We are dealing with this issue unless that one is related? What I do in other tickets is immaterial. I work extremely hard on behalf of drupal.org on a great many different tickets. You fail to mention that at #19 I have updated the IS.

Search existing deprecation notices in the core codebase and you'll find it very normal to state the method being replaced or that none is being replaced.

@shahini_jha Perhaps you should look at what others are saying about me:

I also did some minor edits (mostly just formatting) on the CR, and agree that looks really good.

@oily I was referring to this ticket: # 3497796 📌 Add return type to hook_file_download implementations Active . The comment update happened after the issue reached RTBC, and that is not the scope of the ticket and unnecessary.

I just meant that since the issue is already in RTBC status, if any changes are needed, please feel free to add a comment. I also want to ensure the issue is completed properly, I am trying to complete the issue what i have started. That’s why I am making this request.

RE: #25

@oily I was referring to this ticket: #3497796.

As I stated in #23 issue 3497796.is not related to this issue as a parent, child or in any way at all.

If you want to start a fight about 'you did this on issue arandomticket, and you did that on issue anotherrandomissue I am not interested!

But since you seem to want to knock this back to Needs to Review or is is Needs Work go ahead!

@oily I was just seeking confirmation and discussing it with you. No worries at all—let’s see what others think about this. I’m okay with their perspective, and perhaps I was mistaken. No problem, we can focus on another ticket instead of continuing the discussion here.

RE: #22

@smustgrave Can you please review the last commits? It seems these changes might not be necessary, as this method is not used anywhere.

In that case, I suggest that this notice I have just found in the core codebase contains the message that we need in your MR. (I grepped and found 2x notices stating 'Not needed anymore..')

* @deprecated in drupal:11.1.0 and is removed from drupal:12.0.0. Not
* needed any more.

I suggest that you update the MR and replace my additional text with 'Not needed any more.'

The fact is that someone is going to need to find out why this method is being deprecated. And decide whether it is because it is 'Not needed anymore.' or 'the method has no replacement' because every single deprecation notice in the core codebase of this kind contains such a comment. Or else, they need to state why all the other deprecation notices are including unnecessary detail.

RE: #27

@oily I was just seeking confirmation and discussing it with you. No worries at all—

Oh, good. So if i write a comment directed as @smustgrave stating that your MR should be removed and a new one created and you keep creating MR's that need to be replace, you would be cool with that?

Added comments to the MR.

I have conducted a detailed analysis of existing deprecation notices of methods in the codebase.

There is a pattern evident in the content of every existing notice. They contain two separate types of information.

1. A statement that the method is removed.
2. Further information

The further information states one and one only of the following::

• There is no replacement
• The method or technique or enum that should now be used instead

The MR I encountered contained 'a statement that the method is removed.' It did not contain 'further information'. So I added the 'further information'. This 'further information' I have drawn attention to in my new comments. I am not sure it is correct.

The body of the function should also trigger a deprecation I think. Unlikely that anything is calling this, but if it was, it would tell them.

How to deprecate → explains the steps needed to deprecate something as well as the format of the message. It is in the Core change policies → guide where other useful things about core development can be found. I didn't realize when I last commented here that this is a novice issue, and in that case, this information should have been in the issue summary. At least, in my opinion.

I edited the change record so that it contains only the necessary data. Anyone wanting to know more can follow the link to this issue. While it was well written we also want to be concise, simply stating what changed and any action needed to be taken. In this case there is no action, so it can be short.

This still needs code in the function to trigger a deprecation message. I have updated the issue summary.

Updated the MR to reflect comment #34 and earlier.

Committed/pushed to 11.x, thanks!

catch → closed merge request !10060

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