[RIS] Synonymous keys should be treated the same

Looking at the code in Drupal\bibcite_ris\Encoder\RISEncoder::decode(), I see a lot of problems.

Start with the snippet I already quoted:

      // Year of publication.
      if (isset($record['Y1']) && !isset($fields['Y1']) && !isset($records['PY'])) {
        $record['PY'] = $record['Y1'];
        unset($record['Y1']);
      }

It should be $record['PY'] (singular), not $records['PY']. That is clearly a mistake.

With the default configuration, isset($fields['Y1']) (and similar) will always be TRUE: in bibcite_entity.mapping.ris.yml, Y1 is mapped to ''. So this is dead code.

There are several blocks like this one:

      // Secondary title.
      if ($this->checkKeys(['JA', 'JF', 'T2'], $record) && $record['JA'] === $record['JF'] && $record['JA'] === $record['T2']) {
        unset($record['JA']);
        unset($record['JF']);
      }

In other words, if there is a high degree of redundancy, then remove it. But this does not help at all if the expected key is not set, but one of the alternatives (aliases) is.

I think I am going to radically change this function. We will see what happens to the existing test coverage, and then we will want to add some new tests.

@mark_fullmer:

I can probably fix the missing editors as part of this issue.

The current version is good enough for my current purposes, and for the module with default configuration, but I think it should work harder to respect non-default configuration.

Suppose some site owner noticed that all their RIS files used Y1 instead of PY. Then they might update bibcite_entity.mapping.ris to map Y1 to bibcite_year and un-map PY. The current MR will unset($record['Y1']), so it will not import anything for bibcite_year.

What do you think is the right thing to do? If we want to support all the aliases with standard configuration, then we should pay attention to all of them, even if they are un-mapped in bibcite_entity.mapping.ris. We should certainly make sure that the "last man standing" is one of the mapped keys. But maybe we should ignore the keys that are not mapped.

I guess I am leaning to the idea of looking at all of them, mapped or not, and setting whichever one will be used based on the configuration.

Running the test locally, I noticed a deprecation notice, so I opened #3585052: [Drupal 11] The supportedInterfaceOrClass property is deprecated. It should be pretty easy to fix.

P.S. I am glad that I did that, since otherwise I would have duplicated the effort you just made.

I updated the MR, implementing the change discussed in Comments #10 and #12.

Some tests are failing. I will have to look into it, maybe not until next week.

One thing I do not understand: the author and editor fields seem to be imported even if no RIS fields are mapped to these fields in bibcite_entity.mapping.ris.

@mark_fullmer:

Along with changing ReferenceNormalizerBase::getContributorKey(), we might have to change the structure of the configuration. I added #3585875: Re-think how this module uses bibcite_entity.mapping.* configuration, and I am linking it as a related issue.

I am glad you mentioned that (and that it is out of scope for this issue). It feels like a kluge to be hard-coding all of these keys that show up in the bibcite_entity.mapping.* configuration. I will not worry about it for now, and maybe I will find time to work on it on that new issue.

I think I can fix the problems I mentioned in Comment #13 with a few more lines of code, and I will try to add test coverage by the end of the week.

Another subject: there are a lot of Fixed issues in the queue for this module, and the ones I have checked do not have anyone credited in their contribution records. That means no one is getting issue credit for this work. See Granting credit to issue contributors. In part,

As a project maintainer, it is your responsibility, when marking an issue as Fixed or closed, to grant credit to people who actually helped resolve the issue. Only project maintainers can grant credit for issues in a project.

@mark_fullmer:

I am very sorry. I did not re-check.

I noticed this several days ago ... maybe a week or two. Every issue I checked had an empty contribution record.

Either I was very confused or maybe I checked on a day when d.o was recovering from some recent turmoil (server relocation, data migration). I hope it is the latter, but I cannot be sure.

I added some test coverage, so this issue is ready for review.

In the process, I discovered a couple of problems with the code, which I fixed. I do not claim to follow TDD, but this was in that spirit.

I got some help from Claude Code to set up the test. The problems only came to light after I started tweaking the test "manually".

@mark_fullmer:

No argument here!

Thanks for working with me on this and several other issues. I am glad that you made a new release with all these fixes. Now we can upgrade and drop a bunch of patches that we have been applying.