Automatically declare computed base fields to Views

Working on this.

Not sure best way to test

But why the change to the test file?

> But why the change to the test file?

Because the test no longer needs to declare its computed field to views explicitly. And that's the test coverage -- that field now gets declared automatically.

Thanks for clarifying. In that case the change looks good for next reviewer.

This definitely needs a change record given the API change described in issue summary. I'm wondering if we should trigger a deprecation warning for the cases where a duplicate field is declared? Tagging for subsystem maintainer review in case they'd like to review the API change.

> This definitely needs a change record given the API change described in issue summary.

On it.

> I'm wondering if we should trigger a deprecation warning for the cases where a duplicate field is declared? Tagging for subsystem maintainer review in case they'd like to review the API change.

I'm not sure what we can detect. Existing code might use a different key in the Views data array. Or other code might intentionally duplicate a field declaration.

Done CR.

Nice! Very much +1 on this!

The implicit test coverage is nice and does show that it works, but I would like to see more explicit test coverage here, make sure we are building the data structure we expect, not just a data structure that happens to work.

Also there are a couple of if in there that are untested and especially

if ($field_definition->getType() == 'uri') {
  $views_field['field']['default_formatter'] = 'string';
}

feel like something we should test. Also not sold on hardcoding this here if it is indeed needed. Should we come up with something more API-ish? Doesn't need to be in this issue, but I would like to avoid the horror of a bunch of ifs that do this mapping, we have enough of that for adding normal Fields to the Views data :(

> The implicit test coverage is nice and does show that it works, but I would like to see more explicit test coverage here, make sure we are building the data structure we expect, not just a data structure that happens to work.

Good point.

> Also not sold on hardcoding this here if it is indeed needed. Should we come up with something more API-ish? Doesn't need to be in this issue, but I would like to avoid the horror of a bunch of ifs that do this mapping, we have enough of that for adding normal Fields to the Views data :(

I'm not keen on the hardcoding either, but getting it from mapSingleFieldViewsData() was just too complicated. I started off doing that and realised I'd have to throw away surplus data and possible spoof method parameters too.

The proper more API-ish fix is 📌 Allow @FieldType to customize views data Needs work -- but that issue is nearly 10 years old.

Yeah just adding a @todo linking to 📌 Allow @FieldType to customize views data Needs work would probably be good enough and then hardcode for now ¯\_(ツ)_/¯

Added test and @todo.

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

Rebased.

MR seems have 2 failures that appear to be legit to the issue.

I can't test locally on 10.1 yet as I've not upgraded my PHP. On the 9.5 branch tests pass!

Rebased on 11.x

Still seems to unfortunately have some test failures.

Views kernel tests passing for me locally now.

Ran test-only feature here https://git.drupalcode.org/issue/drupal-3349739/-/jobs/1774681
Which shows the change has coverage.

Applied 2 nitpicky changes, not worth sending back to NW.

So if I'm reading correctly this is just step1 with the other 2 steps marked with todos.

Believe this is good.

I made a few formatting improvements to the CR, and also updated the "introduced in version" to be 11.1.x. 10.3.x is in RC, and 10.4.x will be a maintenance minor.

This is the first issue where I've made the assessment, but I think this API is obscure enough that it is probably not beneficial to backport it to 10.4 for contrib support parity. We can, of course, reopen to address a Drupal 10 backport if it turns out to be an issue for Views-related contrib supporting both Drupal 10 and 11.

Fixed a couple nitpicks and left another open for a fix.

Meanwhile, I'd like this to get a subsystem maintainer review from @Lendude (who reviewed the earlier approach, but not the latest one). That would also be helpful to address the question of whether we need to worry about any sort of deprecation related to the behavior change from #8 or duplicate deprecations (which was posted before @Lendude's last review, but didn't get addressed explicitly at the time. I.e. would the current approach actually result in duplicates and would that cause problems/disruptions if it did?

Thanks in advance. :)

Nice to see this moving!

would the current approach actually result in duplicates and would that cause problems/disruptions if it did?

Yes and yes. Even with just core, this happens. Core only has 2 computed base fields currently, URL alias and Content Moderation state. To test, just use an Umami install with the current MR and you will see these fields get added for nodes. It will also show the problem.
The URL alias field will be there, but won't have any allowed formatters, so you can't make it output anything, which is very confusing.
The Content moderation field will be there twice and you have no idea why and what the difference between the two is.

So it won't break anything, but UX will take a hit as it stands.

I also thought about how we can detect potential problems and for the lack of formatters that might be doable, but the duplicates, I don't see how we could do that reliably. If people are altering things, we have no easy way to detect that, and no way to determine if the change was intended even if we do detect a change.

So some things we could try:
* Check for available formatters before adding the fields (and not add it if there aren't any)
* Expand the description or field name when we add the Views data to indicate that this a computed base field, so there is at least some indication in the UI of where this is coming from

Adding related issue, we're running into duplicate issues there too so maybe we can share some ideas between the 2.