This came up in #3310170: Use UUID as entity ID.
DefaultTableMapping currently needs to combine a bunch of actual "business logic" which determines which field columns go in to which entity table with a bunch of manual array jumbling (diffing/"value"ing/merging/...).
This makes the code less readable than it could be.
-
Replace DefaultTableMapping::setFieldNames() DefaultTableMapping::addFieldNames() and DefaultTableMapping::removeFieldNames().
DefaultTableMapping::addFieldNames() then internally does the "merge -> unique -> value" dance. This allows simplifiying some of the actual business logic. Note that this is never used in any runtime-critical code path so the additional cost incurred by those operations is negligible.
The latter is introduced to bring parity with the current API. It is only used in test code right now, but let's discuss it's future in #3067336: Convert DefaultTableMapping::setFieldNames() and ::setExtraColumns() to protected methods which was opened for that exact discussion.
-
-
-
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
tstoecklerOpened a merge request with the change. This will need to be changed to deprecate
setFieldNames()before it can go in, but I would like some discussion/agreement that this makes sense first, before going ahead with that. Thus, marking "Needs review".Comment #4
quietone commentedComment #5
andypostMaybe it needs to deprecate the old confusing methods?
Comment #6
catchThis looks sensible to me but two things:
1. Agreed with #5 we should deprecate the old method(s) that are being replaced here.
2. There are @todos to make the new methods protected, but they should just be added as deprecated from the start if that's the intention.
Comment #7
tstoecklerRight, as noted in #3 the plan is to deprecate the existing method.
Re marking the methods as protected: I actually disagree with the notion of that @todo, i.e. with what is being pursued in #3067336: Convert DefaultTableMapping::setFieldNames() and ::setExtraColumns() to protected methods. In my opinion we should just remove the @todo and leave the method(/s) public. But since the existing method has that todo it seemed sneaky to just remove it wholesale, just because I disagree with it. On the other hand, it also doesn't seem fair to introduce it as protected right from the get-go if there is no consensus on #3067336: Convert DefaultTableMapping::setFieldNames() and ::setExtraColumns() to protected methods yet.