Hi,

I was just wondering why there isn't a TranslatedNodeProcessor, just like the TranslatedTermProcessor. Figured that it could not be that hard, so I created one and tested it. Works like a charm!

But please, review my patch and will happily apply your feedback.

CommentFileSizeAuthor
#42 interdiff.txt1.9 KBlammensj
#42 add-2752583-42.patch18.08 KBlammensj
#38 interdiff.txt1004 byteslammensj
#38 add-2752583-38.patch19.77 KBlammensj
#34 interdiff.txt1.01 KBlammensj
#34 add-2752583-34.patch18.08 KBlammensj
#32 missing_a-2752583-32.patch18.41 KBborisson_
#32 interdiff.txt5.68 KBborisson_
#30 2752583-30.patch17.52 KBmpp
#28 2752583-28.patch15.86 KBmpp
#23 2752583-23.patch13.37 KBmpp
#20 interdiff.txt1.78 KBmpp
#20 2752583-20.patch7.22 KBmpp
#19 interdiff.txt2.6 KBlammensj
#19 missing_a-2752583-19.patch7.39 KBlammensj
#15 missing_a-2752583-15.patch7.92 KBlammensj
#12 missing_a-2752583-12.patch4.99 KBlammensj
#12 interdiff.txt991 byteslammensj
#9 missing_a-2752583-9.patch4.97 KBlammensj
#9 interdiff.txt1.64 KBlammensj
#6 missing_a-2752583-6.patch4.99 KBlammensj
#6 interdiff.txt637 byteslammensj
#2 missing_a-2752583-2.patch5.1 KBborisson_
#2 interdiff.txt2.51 KBborisson_
facets-translated_node_processor-1.patch5.27 KBlammensj
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

LammensJ created an issue. See original summary.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented
Issue tags: +DevDaysMilan
FileSize
interdiff.txt2.51 KB
missing_a-2752583-2.patch5.1 KB

Hi Jasper,

I found a couple of things that are not up to the drupal standards, I fixed those myself in the attached patch. We'll be committing a few issues today, and I think this'll get in as well. Thanks for the patch!

Status: Needs review » Needs work

The last submitted patch, 2: missing_a-2752583-2.patch, failed testing.

The last submitted patch, 2: missing_a-2752583-2.patch, failed testing.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented

@LammensJ, because the test was no in the correct namespace earlier, it wasn't executed. Can you fix the test fails?

lammensj’s picture

lammensj CreditAttribution: lammensj at iO commented
Status: Needs work » Needs review
FileSize
interdiff.txt637 bytes
missing_a-2752583-6.patch4.99 KB

Yep, my bad.

Status: Needs review » Needs work

The last submitted patch, 6: missing_a-2752583-6.patch, failed testing.

The last submitted patch, 6: missing_a-2752583-6.patch, failed testing.

lammensj’s picture

lammensj CreditAttribution: lammensj at iO commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.64 KB
missing_a-2752583-9.patch4.97 KB

*grmmbl* Drupal using its own deprecated functions... *grmmbl*

Status: Needs review » Needs work

The last submitted patch, 9: missing_a-2752583-9.patch, failed testing.

The last submitted patch, 9: missing_a-2752583-9.patch, failed testing.

lammensj’s picture

lammensj CreditAttribution: lammensj at iO commented
Status: Needs work » Needs review
FileSize
interdiff.txt991 bytes
missing_a-2752583-12.patch4.99 KB

Had some problems with PHPUnit on my laptop. Tests are successful now :)

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented
Status: Needs review » Reviewed & tested by the community

Awesome, thanks!

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented
Status: Reviewed & tested by the community » Needs work

Can we change the current taxonomy term processor to an entity processor and use entity::load instead? If possible useing loadMultiple as well to have less individual loads.

lammensj’s picture

lammensj CreditAttribution: lammensj at iO commented
FileSize
missing_a-2752583-15.patch7.92 KB

The patch includes the all new TranslateEntityProcessor. I didn't replace the previous ones just yet because I'm not sure if this implementation is the way to go... The processor requires that you specify the entity type when selecting it. I needed to add that because EntityTypeManager->loadMultiple asks for one. Makes sense if you think about it, but it was rather hard to find a way to provide this.

I could have loaded the index and retrieved it from there. That should have been the solution in D7, but not in D8 where you can select multiple types of datasource. In other words, take a look at the file and tell me what you think.

Nick_vh’s picture

Nick_vh
he/him
Location Ghent
CreditAttribution: Nick_vh commented

You can actually find it through the index. Since a field cannot be shared across entity types you can look up the entity type in the storage of the field.
So: facet processor -> datasource -> index -> field -> storage -> fetch entity type. Unless I'm missing something here? Discussed this briefly with drunkenmonkey and borisson

lammensj’s picture

lammensj CreditAttribution: lammensj at iO commented

Hi Nick!

I did look in that direction before switching to the configuration form solution. It's just that when you call $facet->getFacetSource(), it will return a FacetSourcePluginInterface object. Can we be sure that this will always be a View?

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented

It won't always be a view, we also support drupal core and search_api_page as facet sources, they don't have that.

lammensj’s picture

lammensj CreditAttribution: lammensj at iO commented
FileSize
missing_a-2752583-19.patch7.39 KB
interdiff.txt2.6 KB

That's what I was thinking... Anyway, the interdiff will show the proposed solution to find the entity type and it works :)

mpp’s picture

mpp CreditAttribution: mpp as a volunteer and at AmeXio commented
FileSize
2752583-20.patch7.22 KB
interdiff.txt1.78 KB

Hadn't seen this issue so I made another try at this in https://www.drupal.org/node/2760401.

I made some small tweaks to #20:
- added . at the end of a comment
- load entity_type the same way as in ListItemProcessor::build()
- don't set setDisplayValue twice

Quick questions though: why do we have three processors (entity, node, term) while they're all entities?

mpp’s picture

mpp CreditAttribution: mpp as a volunteer and at AmeXio commented
Title: Missing a TranslatedNodeProcessor » Missing a TranslatedEntityProcessor
Status: Needs work » Needs review
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented
Status: Needs review » Needs work

Quick questions though: why do we have three processors (entity, node, term) while they're all entities?

That makes sense, it should be sufficient if we only have an entity processor.

mpp’s picture

mpp CreditAttribution: mpp as a volunteer and at AmeXio commented
FileSize
2752583-23.patch13.37 KB
mpp’s picture

mpp CreditAttribution: mpp as a volunteer and at AmeXio commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 23: 2752583-23.patch, failed testing.

The last submitted patch, 23: 2752583-23.patch, failed testing.

mpp’s picture

mpp CreditAttribution: mpp as a volunteer and at AmeXio commented
Assigned: Unassigned » mpp

Guess I'll have to mock the language_manager and entity_type.manager aswell.

mpp’s picture

mpp CreditAttribution: mpp as a volunteer and at AmeXio commented
FileSize
2752583-28.patch15.86 KB

Injected services:
- language_manager
- entity_type.manager

WIP, still needs testing.

mpp’s picture

mpp CreditAttribution: mpp as a volunteer and at AmeXio commented

Mocking all dependencies is tricky. But I noticed $source->getIndex() is based upon SearchApiFacetSourceInterface and thus only available for SearchApiViews, correct?

mpp’s picture

mpp CreditAttribution: mpp as a volunteer and at AmeXio commented
Assigned: mpp » Unassigned
Status: Needs work » Needs review
FileSize
2752583-30.patch17.52 KB
mpp’s picture

mpp CreditAttribution: mpp as a volunteer and at AmeXio commented
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented
FileSize
interdiff.txt5.68 KB
missing_a-2752583-32.patch18.41 KB

I fixed some coding style things and added extra documentation.

This looks great overall. I'd love to hear from @LammensJ if this also works in his usecase, if it does, we can commit this.

mpp’s picture

mpp CreditAttribution: mpp as a volunteer and at AmeXio commented
Title: Missing a TranslatedEntityProcessor » Add TranslatedEntityProcessor

Thanks for the review!

lammensj’s picture

lammensj CreditAttribution: lammensj at iO commented
FileSize
add-2752583-34.patch18.08 KB
interdiff.txt1.01 KB

Too bad we have to rely on the SearchApiFacetSourceInterface class to get the entity type. I guess that's an api change in Search Api if we want to remove that if. @borisson_, @Nick_vh, correct me if I'm wrong.

I also removed the switch-statement for rendering the title. Since every term or node descends from the Entity class, we can just use $object->label().

Furthermore, it works in my use case. No errors and no warning, so good job!

Status: Needs review » Needs work

The last submitted patch, 34: add-2752583-34.patch, failed testing.

The last submitted patch, 34: add-2752583-34.patch, failed testing.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented

Too bad we have to rely on the SearchApiFacetSourceInterface class to get the entity type. I guess that's an api change in Search Api if we want to remove that if.

That is too bad, I agree. But this is not because of search api, but because we also support creating facets for drupal core. And because core's search doesn't have an "index" concept we have write the check.

I'll have a look at the test-failures tonight and when those are resolved, we can commit this. Thanks for the review and for removing the switch @LammensJ, that looks great!

lammensj’s picture

lammensj CreditAttribution: lammensj at iO commented
FileSize
add-2752583-38.patch19.77 KB
interdiff.txt1004 bytes

The tests broke because of the getTitle- and getName-usages which were removed.

lammensj’s picture

lammensj CreditAttribution: lammensj at iO commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 38: add-2752583-38.patch, failed testing.

The last submitted patch, 38: add-2752583-38.patch, failed testing.

lammensj’s picture

lammensj CreditAttribution: lammensj at iO commented
FileSize
add-2752583-42.patch18.08 KB
interdiff.txt1.9 KB

Wrong patch source, my bad.

lammensj’s picture

lammensj CreditAttribution: lammensj at iO commented
Status: Needs work » Needs review
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented
Status: Needs review » Reviewed & tested by the community

Awesome, committing this later today.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and commented
Status: Reviewed & tested by the community » Fixed

Committed, thanks everyone!

Status: Fixed » Closed (fixed)

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