Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Currently, enabling the HAL module breaks the recreation of captured entities.
Proposed resolution
The easy solution is to change 'hal_json' to 'json' in the normalize() calls when capturing, I think. But HAL introduces identification of entities by URI rather than ID, a concept which would be very useful in Collect as well. So possibly we should rather add support for HAL in the recreation flow.
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#6 | support_hal-2462293-6-interdiff.txt | 1.92 KB | mbovan |
#6 | support_hal-2462293-6.patch | 15.31 KB | mbovan |
#5 | support_hal-2462293-5-interdiff.txt | 6.67 KB | mbovan |
#5 | support_hal-2462293-5.patch | 15.29 KB | mbovan |
#3 | support_hal-2462293-3-interdiff.txt | 14.74 KB | mbovan |
Comments
Comment #1
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedAs
hal
module provides URIs of referenced entities (of captured entity) inside_embedded
element and not as a separate values (like JSON does) it requires us to make changes in our plugins. We have to parse_embedded
to get the data we previously had as elements in values container (e.g.uid
...). Then, we need to create property definitions (with typestring
) for those fields that don't exist in values container (only in fields definition container) and set the data from parsed_embedded
element. Doing this, we are able to use URIs instead of IDs as values of reference fields.There is a problem with some fields (e.g Node ID, Revision ID) which are ignored by
hal
, so we don't have that data captured. Anyway, during recreation of entities we skip those values, so maybe we can think about skipping them even in the entity capture process.Because we display the URIs instead of target IDs, I modified
CollectDataFormatter
to display a link to the captured referenced entity container if it exists / (if it is captured). In my opinion, that gives us a better UI for showing relationships between containers. Also, I added web tests for this UI feature.Comment #2
miro_dietikerThis whole issue leads us to challenging problems and i'm not yet sure about the design implications and decisions to take.
We discussed that:
- We don't like that Hal removes values and we lose information through this. See also #2304849: Stop excluding local ID and revision fields from HAL output
- Field definitions provided by a client or anyone should expose the data representation and not the internal form. Thus entity_references are represented as URIs and need to be described as URIs/strings.
Hal and how it deals with references is strongly related to our support for relationships. We don't have that yet so we are at the beginning of the related learnings.
Berdir mentioned that we possibly need consider implementation of an own serializer to follow our rules / expectations.
I will create followup issues about the (many) identified related problems...
Comment #3
mbovan CreditAttribution: mbovan at MD Systems GmbH commented- Changed creation of
DataDefinition
to type URI for fields that have a typeentity_reference
/file
/image
.- Enabled
hal
forcollect_client
.- Fixed output for
Uri
data definitions.- Fixed
FieldDefinitionSchema
plugin output.- Fixed tests.
The
nid
/rid
values are skipped, ashal
doesn't provide that kind of data.As now, if we want to compare field definitions with the current system we will get different results (even if there are no missing fields/bundles) because we have different structure. The question is, do we want to convert current field definitions data to match the structure we are capturing or to leave like this so we can clearly see which fields are "different"?
Recreation of entities is half-broken. There are no visible errors, but entity reference fields are skipped.
But, creation of missing fields is broken in case we want to create those fields that we don't have data/settings for (e.g.
field_image
).Comment #4
ArlaThis huge line could use some splitting and/or comments to clarify what is happening. Also, could we extract it to a collect_common function? Because we use it on both sides.
The
([
are on the same line, so the])
should also be.Unused
I think the checkPlain will encode some URLs, especially with
&
in them, so probably better to check UrlHelper::isValid() first, and if not a URL, checkPlain it. (As the comment says.)This comment was a little bit confusing to me. I think this at least needs a description of what kind of data is in _embedded.
Why the change of the create() params? If this causes problems, I think we can just remove the $container_entity_adapter and leave the 3rd and 4th parameters empty.
The only difference between the cases is the class name, maybe we could move the denormalize call outside the if-else
I'm also not really sure what we want. This is related to what @miro_dietiker said in #2: "We don't like that Hal removes values and we lose information through this." Let's discuss in a followup?
Comment #5
mbovan CreditAttribution: mbovan at MD Systems GmbH commented@Arla:
1. Tried to move it in collect_common module but it requires some changes in service calls. We need to decide what services we want to inject (and where), where to put the new service, or on the other side implement it as a new global function in collect_common. I guess this can be done in a follow-up. Until then, I added comments which explains this long line on both sides (client and server).
2. Changed.
3. Removed.
4. Changed.
5. Changed the comment.
6. It caused some errors because of missing
name
parameter, so we added both parameters. We don't have same problems if both parameters are empty, so I removed$container_entity_adapter
.7. Fixed.
Comment #6
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedRenamed variables, removed parameters which are by default empty.
Comment #7
ArlaLooks great :D
Committed and pushed!
So, followups needed to make sure recreation of ER fields works, to decide on what to diff, and resolving URIs when recreating ER values. To tackle some of this, @miro_dietiker suggested changing the field definition serialization (for creating FD containers) to provide the URI-list definitions in parallel with all field definitions. Thus we could use the field definitions for diffing and creating fields, and the URIs with other standard Collect usage.
Comment #9
ArlaCreated followups:
#2480109: Field definition container: include ER fields as well as URIs
#2480103: Resolve URIs to recreate entity reference values