Move text_summary to TextSummary service and deprecate

The helper class looks good to me. I think the last piece of this is to update the change record and issue summary. I suggest that you point out that the new function is internal. It took me a minute to catch on.

Got a couple suggestions, we gotta do removals in 13 at this point.

I think this looks good.

I think we can improve this, posted a review.

I only saw now that the suggestion is to make this internal, but I'm not sure. What exactly is internal even in the scope of core? there is a call in node module for example. I'd argue that internal is something that should only be used within the same module or namespace.

It's a very hard thing to search for in contrib, matches lots of strings and module names and what not, but I think there's value in this being a non-internal API.

The number of test fail seems pretty limited and a clear indication that the current docs are incomplete and not correct:

TypeError: Drupal\text\SummaryGenerator::generateSummary(): Argument #1 ($text) must be of type string, Drupal\filter\Render\FilteredMarkup given, called in core/modules/text/tests/src/Kernel/TextSummaryTest.php on line 356

I think if you allow string|\Stringable then that should work.

It's much easier to make sure we have the correct type and docs now rather than change an existing method later.

This is ready again, I converted it to a service removed static, added DI and fixed typing.

I think it's better as a service, thanks. The one remaining bit for me is to decide whether we want to follow my suggestion of the size default config and not support that from the start in the new service. As mentioned, it's easier to define the size parameter as required now and keep the config in the BC layer as opposed to changing it later. As far as I see, there are no tests or calls that rely on the implicit value right now, so there shouldn't be any changes to the current calls?

I'm not sure why we would drop the size config, is it to just remove the config dependency?

See https://git.drupalcode.org/project/drupal/-/merge_requests/14416#note_67..., @smustgrave wants to remove that setting and whole config object entirely. It's unused in core as $size is never NULL and it has no UI. My suggestion might be the easiest way to get there.

What about this as a compromise here?

We drop passing in null to the new service, we drop reading the config and we handle the config cleanup and dropping it here: #3436677: Remove text.settings config since it's only used in 1 place

To bike shed a bit:

SummaryGenerator?

It's repetitive, but the method name could also be generateSummary

What about TextSummary::generate? Or do we want the word generator in the class? Feel free to ignore this comment as I don't have particularly strong feelings here.

That is better naming, I pushed it.

A couple small comments on the MR. I also took a pass at assigning credit and made small edits to the CR.

FYI, in kernel tests, Drupal::service() is generally safer than $this->container->get(), because $this->container->get() can point at outdated objects after a container rebuild.

In Functional tests, the use of either is essentially equivalent since #2066993: Use magic methods to sync container property to \Drupal::getContainer in functional tests.

Tried to address the feedback but adding delcare=strict broke a bunch of tests is that needed at this moment or could be a follow up?

The thing is that if we don't do it now, it'll be more difficult to do later.

I think we can do it here with a few minor changes. Made the suggestions on the MR.

I did a fresh complete review since a lot has happened since I last looked at this. I commented on a few things. I have a concern about the generate() function's return type and I left a couple of minor suggestions on the test.

The test failure looks random. LGTM.

I had the same failing test, opened #3578666: Fix AssetAggregationAcrossPagesTest

The test failure is upstream #3578653: AssetAggregationAcrossPagesTest is broken.

I think this looks good, but will wait for that one to get in.

#3578653: AssetAggregationAcrossPagesTest is broken is in. Can someone merge in latest main to get the tests run updated?

Committed f929b61 and pushed to main and acf3e5f and pushed to 11.x. Thanks!