There are several places in our code where we use one of the entity_load*() methods directly, which are now all deprecated. These should be either replaced by a dependency-injected variant (where possible – e.g., the content entity datasource), or $entity_type_class::load() (where the type/class is known), or by going through the entity manager service directly (all explained in the respective @deprecated descriptions for the functions).

Comments

drunken monkey created an issue. See original summary.

lhangea’s picture

Assigned: Unassigned » lhangea
lhangea’s picture

Status: Active » Needs review
StatusFileSize
new9.79 KB

Status: Needs review » Needs work

The last submitted patch, 3: 2547747-3.patch, failed testing.

lhangea’s picture

Status: Needs work » Needs review
StatusFileSize
new9.79 KB

Status: Needs review » Needs work

The last submitted patch, 5: 2547747-5.patch, failed testing.

lhangea’s picture

Status: Needs work » Needs review
oenie’s picture

I think you need to make sure you generate your patch in the root drupal directory, to make sure it is properly generated.
The testrunner is tripping over the paths in your patch, which are relative to the directory of the search_api module.

lhangea’s picture

StatusFileSize
new9.96 KB

Status: Needs review » Needs work

The last submitted patch, 9: 2547747-9.patch, failed testing.

oenie’s picture

Are you inside one of the rooms in Barcelona ?
If you feel like it, i could come by. I'm in the General Sprints room.
Let me know which room/table :)

lhangea’s picture

StatusFileSize
new10.63 KB
lhangea’s picture

Status: Needs work » Needs review
lhangea’s picture

Assigned: lhangea » Unassigned

The last submitted patch, 3: 2547747-3.patch, failed testing.

The last submitted patch, 5: 2547747-5.patch, failed testing.

The last submitted patch, 9: 2547747-9.patch, failed testing.

drunken monkey’s picture

StatusFileSize
new19.46 KB
new9.89 KB

Thanks a lot for your work here, this already looks excellent, and you also seem to have caught all of them!

Just a few minor details:

  1. +++ b/search_api_db/src/Plugin/search_api/backend/Database.php
    @@ -2034,11 +2034,6 @@ class Database extends BackendPluginBase {
    -    // Make the input lowercase as the indexed data is (usually) also all
    -    // lowercase.
    -    $incomplete_key = Unicode::strtolower($incomplete_key);
    -    $user_input = Unicode::strtolower($user_input);
    -
    

    That seems to be left over from an earlier patch?

  2. +++ b/src/Plugin/search_api/datasource/ContentEntity.php
    @@ -523,8 +523,9 @@ class ContentEntity extends DatasourcePluginBase {
    +    $index_storage = $this->getEntityManager()->getStorage($this->getEntityTypeId());
    

    There is actually already $this->getEntityStorage() for that in this class.

  3. +++ b/src/Tests/IntegrationTest.php
    @@ -34,6 +34,21 @@ class IntegrationTest extends WebTestBase {
    +  protected $index_storage;
    

    Now this is just my pedantic nit-picking about coding standards, but property names should actually be camelCase, not snake_case.
    But good idea to separate this into its own property instead of having that code in numerous places.

  4. +++ b/src/Tests/IntegrationTest.php
    @@ -172,7 +188,8 @@ class IntegrationTest extends WebTestBase {
    +    $this->index_storage->resetCache([$index2_id]);
    

    Also coding style nit-picking, but even more specific: we're currently not using the shorthand array syntax in the Search API, and I'd like to remain consistent in that aspect.

Attached is an updated patch with those few problems removed. Also, while we're at it, I also went ahead and removed all the other calls to entity_*() functions (which are now all deprecated, it seems).
Sorry, didn't notice that earlier.

Status: Needs review » Needs work

The last submitted patch, 18: 2547747-18--remove_entity_function_calls.patch, failed testing.

The last submitted patch, 18: 2547747-18--remove_entity_function_calls.patch, failed testing.

drunken monkey’s picture

Status: Needs work » Needs review
StatusFileSize
new18.74 KB

Forgot to remove the left-over database backend change in the previous patch.

Status: Needs review » Needs work

The last submitted patch, 21: 2547747-19--remove_entity_function_calls.patch, failed testing.

The last submitted patch, 21: 2547747-19--remove_entity_function_calls.patch, failed testing.

drunken monkey’s picture

Status: Needs work » Needs review
StatusFileSize
new18.71 KB
new927 bytes

That's the kind of stuff that happens if you try to be clever at midnight …

Status: Needs review » Needs work

The last submitted patch, 24: 2547747-24--remove_entity_function_calls.patch, failed testing.

The last submitted patch, 24: 2547747-24--remove_entity_function_calls.patch, failed testing.

lhangea’s picture

All right, I think I found the source of the exception.

lhangea’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 27: 2547747-27--remove_entity_function_calls.patch, failed testing.

lhangea’s picture

Status: Needs work » Needs review
StatusFileSize
new18.74 KB

I missed a parenthesis.

Status: Needs review » Needs work

The last submitted patch, 30: 2547747-30--remove_entity_function_calls.patch, failed testing.

lhangea’s picture

Status: Needs work » Needs review
StatusFileSize
new18.74 KB

@oenie yes I was at DrupalCon sprints, sorry I didn't see your comment from #11.

The last submitted patch, 27: 2547747-27--remove_entity_function_calls.patch, failed testing.

The last submitted patch, 30: 2547747-30--remove_entity_function_calls.patch, failed testing.

drunken monkey’s picture

Status: Needs review » Fixed

Great job, thanks! And you even changed the property name I forgot to align with the coding standards, nice.
So, committed.
Thanks a lot again!

Status: Fixed » Closed (fixed)

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