The pr is missing `ActionAccessControlHandler.php` which I will add shortly
@smustgrave,
Opened a pr and tried to fix some cs + php compatibility issues.
Don't rly know what this means
Trait Drupal\Tests\PhpUnitCompatibilityTrait referenced with incorrect case: Drupal\Tests\PhpunitCompatibilityTrait.
Also I did not rly run the tests, will do at some point if noone else does it first, to fix another issue with willreturnValueMap
Sorry for posting after this was closed.
Has this not been merged to 2.x, Or am I doing something wrong?
Ok so in the patch, that I attach, I got quite a few suspect things, so I will start with some disclaimers.
Disclaimer 1: Please do not use this patch in production, unless you are sure it will not cause you any of the problems I will mention below. I have on purpose hidden the patches from display.
Disclaimer 2: Some existing tests will fail, I have not taken the time to maintain them yet, since I would like some feedback first.
With those out of the way, here is what I did.
@qzmenko I based my patch on #44 and not on #47 because it is not clear to my when/why you would have a non existent entity if a reference to it exists.
I am sure there are valid cases, I just don't have any in mind atm, and I am not sure how we should handle those cases. In #47 you simply let the FieldHelper do as it would before, which means index the fields normally, which makes no sense to me if the referenced entities, for said field, do not exist.
@mkalkbrenner With #44 which is basically #41, The described functionality worked fine for nested field variations:entity:whatever:whatevercode but not for direct fields e.g. as simple taxonomy field field_brand.
In my understanding, the first time extractFields is called, the ComplexDataInterface $item is the actual indexed entity. We don't care about that since it will be filtered out later, or should have already been filtered by the actual processor (Not sure atm at what point this happens, but does not rly matter). Then for that entity, the selected fields to index are split between nested and direct fields. For nested fields we have a recursive call, which then filters those fields based on status.
For direct fields we don't do that, instead extractField is called. I extracted your logic in a helper function and used it in extractField as well... and here I had some thoughts.
@drunken monkey I refrained from injecting the $index variable to the extractField function, since it already has FieldInterface $field as an argument, and we should be able to extract the index from there. I had to pass the index to the extractFieldValues function though, which does the actual work. Now here I got some issues.
This funciton is used by stuff I could not test atm in my code, and I think would have weird side effects.
E.g. It is used in AddHierarchy processor. I don't know what will happen if you stop indexing something that has published children. How do we handle the children? It would make sense to stop displaying them if their ancestor is not displayed, but maybe some would argue that it's ok to keep showing them without the ancestor. I don't know how something like that is already handled in the processor, and do not atm have a working environment with matching data to test this, so I made the index variable nullable in extractFieldValues and my helper function, which leads extractFieldValues calls to gracefully keep doing what they were doing until now.
Maybe I am overthinking it, maybe not, idk.
Same for the SearchApiFieldTrait::898 in `extractPropertyValues`... Did not handle that, since I am not sure If we want to.
We could either decide for them here, or handle those cases in their separate follow ups, I am not so familiar with the entire codebase yet.
Lastly I completely ignored existing tests for the extractFieldValues, until we decide if this is a right approach. Give me some feedback, and I could get to them after.
TLDR:
- Stuff on #47 were not clear to me, we should address this point, but I left it our for now.
- Direct fields were not handled by the proposed logic only nested... Think I cover this in the attached patch now.
- Did not cover every call of the extractFieldValues function, to avoid possible side-effects.
I agree with @mkalkberenner that the solution proposed in the linked issue is better.
Is there any reason to keep this issue open, or could we fall back to the other one and close this?
Main thing I'm worried about here is duplicating a product with thousands of variations
This probably is a more general issue, as you mentioned, and maybe it's best to be tackled on a separate issue.
This potentially has to be improved btw
+1
Marios Anagnostopoulos → made their first commit to this issue’s fork.
I can confirm that this is not an issue for me any more in 3.0.
Attaching a patch based on #24 for 2.37
Upgrading to gin/toolbar 1.0.0-rc4 fixed it for me, so I guess it is not a core issue and this thread can be closed?
Attaching a patch for 10.2.2 (Based on !4423) (Probably applies to older versions as well, that #97 does not cover)
Marios Anagnostopoulos → changed the visibility of the branch 2551893-add-entity-events to hidden.
Descr WIP
Queued for test and changing to needs review.
Adding a patch with the array return type as suggested by the OP
Here is an interdiff
Re-uploaded Patch 4 using the spaceship operator and adding php 8.2 support as per https://www.drupal.org/project/facets/issues/3360426 📌 PHP 8.2 compatibility Fixed
Rerolled for 3.6
Would it make sense to create something like a comparison table for the HUX/Event implementations?
Also I am not sure If I am the one who should mark the gitlab comments as resolved or the reviewer :)
I made both the changes requested, changing it back to needs review.
Marios Anagnostopoulos → made their first commit to this issue’s fork.
I removed the evend name from the dispatching since we are using separate events for the import/export processes and added a brief documentation for the import event.
I remove the changes in the drush.inc files, since we discussed that they will be removed anyway in another issue.
I also abort the execution of the command, if no entity id is provided but properties were given.
I ended up not throwing an exception, since the check is made in the command itself, so I just stop the execution after displaying an error via the logger.
If this is not the best way to go about it, let me know and I'll change it.
Forget !4340...
I moved the changes of !210 to 11.x in !4423
I will start working on the review of @claudiucristea, unless someone beats me to it, or the issue ends up getting dropped.
Having said that.. has there been any discussion on the matter?
@mkalkbrenner I guess the approach should change now since the views_ajax_get is redundant?
Then again how would we handle BC for < 10.1.x sites?
Here is the updated patch for anyone who needs it
I created a pr with the changes from the auto created patch.
Also I added a type hint in RouteSubscriber::getSubscribedEvents to fix the issue of not being able to install the module in D10
Marios Anagnostopoulos → made their first commit to this issue’s fork.
Re-uploading the same patch with the correct name.
Also adding an interdiff.
Here is a reroll for 9.5.10 for anyone who might need it.
Leaving is as needs review until someone posts a patch for at least 10.x
You should have used it like so https://git.drupalcode.org/project/drupal/-/merge_requests/1943/diffs.patch
Preferably download it and use it as a local patch in your composer.json since, if the code in the pr changes, the patch will change as well with your next composer install/update whatever, and that might be something you don't want to happen uncontrollably.
I uploaded a patch you can use, and it will also be useful (probably) for allowing drupal.org test suite to run.
Hello there,
I know that lucene backends provide ways to request the results pre-wrapped.
Besides that though, the DB backend (and probably others as well) don't, so that's a nice addition.
With Drupal's theming principles in mind, the patch is sound and it should remove (minor but still) headaches when needing to theme the highlighter results.
All in all, looks good.
Changing this to `needs work` since an issue was reported in #19
I created an issue fork and pushed the changes up to #37 which apply to 10.1.x
I didn't include the tests provided #54 since the patch in that comment was based on the older implementations (as per #60) and because I was not sure if they would be relevant (as per #44)
If you think that they should be included, feel free to commit and update the issue.
The interdiff I provide is just the missing one between 29 and 37.
9.5 has some js test failure (AFAI can understand) and the second run passed.
Is there anything else to be done?