remove inner _add_ajax_listeners_to_plugin_inputs and _add_attachments_to_editor_update_response function definitions

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

I respectfully disagree with #8. I believe this is caused by a slight misunderstanding 😊

I disagree not because of the rules of "beta experimental" (those are fine! 👍), but because I think #8 overlooked the argument that #6 made: #8 only responded to #6's 3rd point, not the first 2. @neclimdul's 3rd point was a mere "by the way, this too!" — and that one was inaccurate and hence seems to have prompted #8.

This does not fall under the BC policy because:

1. It's underscore-prefixed, meaning it's internal.
2. It's only accessible to anybody else if:
   1. the contrib/custom code calling it does so only on /admin/config/content/formats/manage/{filter_format}
   2. the contrib/custom code calling it explicitly calls ckeditor5_form_filter_format_form_alter() outside of the context of /admin/config/content/formats/manage/{filter_format} to trick it into defining _add_ajax_listeners_to_plugin_inputs().

Finally: what viable reason could any code possibly have to call _add_ajax_listeners_to_plugin_inputs()? Because it specifically adds an #ajax callback that is also internal: _update_ckeditor5_html_filter().

So, assuming this was a simple misunderstanding: rerolled @neclimdul's patch and re-RTBC'd.

Custom Commands Failed, phpstan baseline needs to be updated.

Not a misunderstanding; please review: https://www.drupal.org/about/core/policies/core-change-policies/drupal-d... →

Adding a deprecation is easy and harmless. And @neclimdul found it somehow? I would feel differently if the function were broken, or harmful, or difficult to deprecate cleanly but as it is it's just unnecessary API surface that we can get rid of in like 6 months anyway when we'll presumably open 11.0.x. I won't die on this hill, but internal APIs -- even silly ones -- are given deprecations. It doesn't need a test or any of that nonsense, just @deprecated with the boilerplate in the linked example. :)

Well, I clearly had some misunderstandings. 🤣 Wim was right though, 3 was a "and if that's not enough" though.

> And @neclimdul found it somehow?
A fair assessment because I'm usually fighting _for_ BC in something that broke my site 😁. Its been a year but pretty sure in this case I was just working through the phpstan baseline at the time and this code looked really gnarly and need of some simplification. I'm definitely _not_ doing anything anywhere with this code and am not convinced you really could use it.

Just to make sure we're all on the same page because the patch doesn't make it super obvious and I don't think I've ever seen this in core, I want to re-enforce these are functions dynamically defined inside another function. They are not just your normal function in a file which I would consider an API. e.g. https://3v4l.org/Pd4f3

Because of the just really weird leaky nature of this code, I'm not sure this is an API as such but just leaking scope. If it is an API, I feel confident we'd all consider this internal so lets look at that.

As the patch is implemented, providing BC is pretty troublesome because we're optimizing the logic to entirely exist inside the scope of the primary function which means you can't also have a global function that sharing the same code. I think the only way to provide that BC is to use approach #2 from #2 of making this an actual internal API of the original code but also immediately deprecating it which a @TODO of "remove this and apply approach 1" in the future.

I don't like that but... I guess?

reroll the current patch with the phpstan baseline fix but also fixing a conflict from 📌 Replace most strpos() !== FALSE or === FALSE with str_contains() Fixed to see if we're still in a good place with testbot.

+++ b/core/phpstan-baseline.neon
@@ -825,21 +825,6 @@ parameters:
-			message: "#^Inner named functions are not supported by PHPStan\\. Consider refactoring to an anonymous function, class method, or a top\\-level\\-defined function\\. See issue \\#165 \\(https\\://github\\.com/phpstan/phpstan/issues/165\\) for more details\\.$#"

BTW, I think this exact line is what lead me to file this. As you can see the two approaches in the original patches are the two applicable solutions in the PHPStan error. :-D

Moving to HEAD/main/10.2.

Ohh you are totally right about the scope here; thanks @neclimdul. I also clearly forgot what this issue was even about over the past year and a half. I am convinced and agree -- no deprecation. Sorry for making you explain yourself twice (or three times, counting Wim's).

Hah, no worries. I had to do so reading to remember what was going on too :-D

Since concerns are addressed, things are green again, and we've got the same patch as a previous RTBC(with the str_contains conflict resolution), going to optimistically bump it back to RTBC.

OK confusing issue but second time reading the comments and first time reading the patch it looks fine.

Committed/pushed to 11.x, thanks!

🥳

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