Problem/Motivation

We added pre-epoch date support on #3189051: Handle pre-epoch dates, however on 8.0.x we don't have full_date data type.

Proposed resolution

Either:

  1. Add full_date data type to 8.0.x
  2. Add on existing update hook the task to convert full_date to "string" (see: #3189051-16: Handle pre-epoch dates)

Remaining tasks

  1. Write initial patch (done by @ckontakis in merge request !89)
  2. Add tests - done by @kdborg and @mparker17 by #11
  3. Review and feedback - done by @mparker17 in #10
  4. RTBC and feedback - done by @mparker17 in #10
  5. Commit - done by @mparker17 in #12
  6. Release - released by @mparker17 in 8.0.0-alpha5

User interface changes

Adds a new field type, Full Date (machine name elasticsearch_connector_full_date) to the Index's field configuration UI.

API changes

Adds an @SearchApiDataType with ID elasticsearch_connector_full_date and makes it available to the field mapper.

Adds an update hook (elasticsearch_connector_update_9804()) that changes fields of type full_date into elasticsearch_connector_full_date fields instead.

Data model changes

One-time changes fields of type full_date into elasticsearch_connector_full_date fields instead via update hook.

CommentFileSizeAuthor
#6 elasticsearch_connector-full_date_data_type-3470901-6.patch1.48 KBkdborg@gmail.com
#4 elasticsearch_connector-full_date_data_type-3470901-4.patch2.56 KBkdborg@gmail.com

Issue fork elasticsearch_connector-3470901

Command icon Show commands

Start 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:

About issue forks

Comments

sokru created an issue. See original summary.

ckontakis made their first commit to this issue’s fork.

ckontakis opened merge request !89

kdborg@gmail.com’s picture

kdborg@gmail.com commented
StatusFileSize
new elasticsearch_connector-full_date_data_type-3470901-4.patch2.56 KB

Here's a patch for the changes proposed above. This works with both the local elastic search instance and an ElasticCloud instance using the patch here: https://www.drupal.org/project/elasticsearch_connector/issues/3522107#co...

sokru’s picture

sokru commented
Issue tags: +Needs tests

Thanks for working on this! Now that we have almost 1000 installs on 8.x branch, I'd be a little cautious to add a new update hook. The issue summary suggest adding it to existing one, so it would work for people coming from 8.x-7.x branch. People already on 8.x branch have probably done this by custom module.

Also the new full_date plugin does not have same logic as 8.x-7.x branch (see https://git.drupalcode.org/project/elasticsearch_connector/-/commit/b4f2...).

We should add tests for this feature.

kdborg@gmail.com’s picture

kdborg@gmail.com commented
StatusFileSize
new elasticsearch_connector-full_date_data_type-3470901-6.patch1.48 KB

I've removed the hook_update from the install file. See attached.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Status: Active » Needs work

The patch looks good, but as @sokru pointed out, it needs tests. Automated tests ultimately benefit you (and your client(s)): they ensure future changes will not break the functionality your site depends on.

If you need help writing tests, please ask us!

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes

Updating issue summary

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes

Link to merge request in issue summary.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented

Testbot likes it except for a PHPStan lint, and my manual testing shows it is working, so this works for me: let's fix the PHPStan lint and see if we can add a test.

(additional context: @kdborg and i are pair-programming on this issue in real time)

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs tests

I've added a test now, so I think I'm going to promote this to RTBC.

  • mparker17 committed 50b38070 on 8.0.x authored by ckontakis 
    [#3470901] feat: Upgrade path for full_date data type

By: sokru
By:...
mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Merged! Thank you very much everyone!

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Category: Bug report » Feature request

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

mparker17’s picture

mparker17
he/him
Primary language English
Location UTC-5
commented
Issue summary: View changes

Released in 8.0.0-alpha5!