Ease inheritance of ContentEntity datasource

Created on 4 September 2024, 13 days ago
Updated 14 September 2024, 2 days ago

In search_api_default_content_deploy we extend the ContentEntity Datasource and just overwrite the getItemId() method.
Unfortunately getPartialItemIds() has it's own redundant logic to build the item ID.

I think getPartialItemIds() should leverage getItemId() instead for a better DX.

✨ Feature request
Status

Fixed

Version

1.0

Component

General code

Created by

πŸ‡©πŸ‡ͺGermany mkalkbrenner πŸ‡©πŸ‡ͺ

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @mkalkbrenner
  • Status changed to Needs review 12 days ago
  • πŸ‡©πŸ‡ͺGermany mkalkbrenner πŸ‡©πŸ‡ͺ
  • Pipeline finished with Failed
    12 days ago
    Total: 365s
    #273238
  • Pipeline finished with Failed
    12 days ago
    Total: 336s
    #273294
  • πŸ‡©πŸ‡ͺGermany mkalkbrenner πŸ‡©πŸ‡ͺ

    The API is inconsistent.
    getItemId() checks for the current config.
    getItemIds() doesn’t.
    So getItemId() is not suitable for deletions caused by config changes.

    I consider that to be a bug.

  • Pipeline finished with Success
    12 days ago
    Total: 326s
    #273363
  • πŸ‡©πŸ‡ͺGermany mkalkbrenner πŸ‡©πŸ‡ͺ

    My initial approach was to call getItemId() from getItemIds() but due to the different behaviour the tests failed.
    But as this is a bigger issue I now introduced formatItemId() to have something small to overwrite.

  • Pipeline finished with Failed
    11 days ago
    Total: 324s
    #274569
  • Pipeline finished with Failed
    11 days ago
    Total: 295s
    #274586
  • Pipeline finished with Failed
    11 days ago
    Total: 347s
    #274596
  • Pipeline finished with Success
    11 days ago
    Total: 2693s
    #274612
  • Pipeline finished with Failed
    11 days ago
    Total: 341s
    #274702
  • Pipeline finished with Success
    11 days ago
    Total: 348s
    #274756
  • Pipeline finished with Success
    11 days ago
    Total: 310s
    #274786
  • Pipeline finished with Success
    11 days ago
    Total: 334s
    #274809
  • πŸ‡¦πŸ‡ΉAustria drunken monkey Vienna, Austria

    drunken monkey β†’ made their first commit to this issue’s fork.

  • πŸ‡¦πŸ‡ΉAustria drunken monkey Vienna, Austria

    Thanks a lot for suggesting this! The changes do make sense, great if this helps other modules provide their own entity-based datasources.
    I had a few remarks and pushed the associated suggested changes. If you are happy with those, I’ll merge.

  • πŸ‡©πŸ‡ͺGermany mkalkbrenner πŸ‡©πŸ‡ͺ

    Looks good to me. I'll test the latest version of the patch on Monday.

  • Status changed to RTBC 7 days ago
  • πŸ‡©πŸ‡ͺGermany mkalkbrenner πŸ‡©πŸ‡ͺ

    I adjusted the code in search_api_default_content_deploy to the latest changes and it works :-)

  • Status changed to Fixed 2 days ago
  • πŸ‡¦πŸ‡ΉAustria drunken monkey Vienna, Austria

    Good to hear, thanks for testing!
    Merged.

Production build 0.71.5 2024