- 🇩🇪Germany Anybody Porta Westfalica
We should also have a test for this. It should fail for now and work with the fix. Thank you!
- 🇩🇪Germany mxh Offenburg
@Glottus thanks for your response - have you tried merge request !3 already, or do you mean with "those pieces above" the hints from #13?
When MR3 doesn't work either, feel free to directly set this back to "Needs work".
- 🇺🇸United States smustgrave
With D7 EOL soon going to mark this one as fixed
Automatically closed - issue fixed for 2 weeks with no activity.
- 🇺🇸United States Glottus
Thank you, mxh, my ECA model does include the "VBO: Custom access for Views bulk operation" (I've done this with several other models on other sites successfully) and my view does include the "Global: ECA bulk operations" field". The proposed code edits by jurgenhaas allowed me to move forward on development for now, but I'll continue to test the other issue that I opened, as even with those pieces above, I see the bug of the disappearing checkboxes every once in a while. I'll report back more on that thread if I can offer anything more.
- 🇩🇪Germany mxh Offenburg
Attaching a sample model for a VBO process that works after applying MR !3.
There are some aspects that may sometimes be overseen easily:
- You need to use the "Global: ECA bulk operations" views field plugin within your configured view.
- You need to use the action "VBO: Set custom access on Views Bulk Operation" after the event "VBO: Custom access for Views bulk operation" for granting access.
Probably some more hurdles, I know it's not that easy to set it up correctly.
- First commit to issue fork.
- 🇩🇪Germany mxh Offenburg
I'm able to reproduce the infinite recursion, also with the latest branch of 2.1.x that contains the first fix attempt of this issue.
Please have a look first whether the infinite recursion is fixed for you with merge request !3. Let me know whether that fixes the problem for your or not. Setting to "Needs review" for that one - no matter of the result we still need proper test coverage on this particular problem.
The actual part which is triggering the infinite recursion is here: https://git.drupalcode.org/project/eca/-/blob/2.1.x/src/Entity/Eca.php?r...
That part is extremely bad resource-wise, because it's already loading all plugins when just loading ECA config entities. This must be fixed as well, but it's not part of this issue (belongs into a new issue within the ECA issue queue). It can easily be seen when debugging the recursion with Xdebug. Next time please provide a stack trace / debug stack so we may have a chance to identify this earlier.@jurgenhaas I appreciate your efforts on working on this module. Let's please get through a proper review process next time, instead of directly committing and setting to fixed without merge requests. We could also work with a "timer", let's say after two weeks of no response someone of us may proceed at their own will. For a minimum I'd like to get a chance for reviewing first.
I will update the title and confirm the steps in the updated summary by EOD Monday 2024-07-29.
- @mxh opened merge request.
- 🇺🇸United States smustgrave
Thanks for reporting, as a bug can we get a test case showing the issue
- 🇺🇸United States DamienMcKenna NH, USA
Thank you for updating the merge request.
Now we just need some test coverage to make sure zeroes are handled properly.
Added change record for deprecation.
Pushed a draft MR with the changes, will add tests later.- @godotislate opened merge request.
- 🇩🇪Germany mxh Offenburg
It may happen when both state and plugin cache need to be rebuilt at the same time.
Could you please tell what is a "state plugin"?
I'm not yet convinced this is the right solution. Before continuing on this, we need a test that will reproduce this. Once we have a proper test that reproduces this correctly, then we can try to find a proper solution. Otherwise this is just a guess game.
A second indicator this is not the solution yet is the reported problem in 🐛 Edits to model appear to break configuration in View Active which is the first time I see that the problem got reported. But this module is around for quite some time now, with more than 100 site installations. Some other user would already have gotten to the same problem and reported it already. But now you have the problem and reported it at the time you tried the current MR of this issue.
Besides that, please don't create new issues already that are coming from other issues that are not yet fixed.
At least for now, we need to work towards a proper test. If you don't know how to do it no problem, maybe you can at least provide a configuration export (no whole site export, just the relevant ones) together with a minimal list of steps, that will reproduce your problem.
Hi, at the patch #41 the part:
+++ b/core/modules/views/tests/src/Kernel/Entity/EntityViewsDataTest.php @@ -513,6 +513,10 @@ public function testDataTableFields() { 'value' => 0, 'numeric' => TRUE, ],
doesn‘t match my version:
public function testDataTableFields() { $entity_test_type = new ConfigEntityType([ 'class' => ConfigEntityBase::class, 'id' => 'entity_test_bundle', 'entity_keys' => [ 'id' => 'type', 'label' => 'name', ], ]);
Including all the following changes.
I‘m running Drupal 10.1.0. Does the version 10.1.1 make that difference?regards
- 🇺🇸United States mherchel Gainesville, FL, US
The
3250234-back-to-top-mikes-sdc-branch
above is ready for review. I added a JS fallback for older browsers and fixed some linting.Note the scroll progress only works in Chromium as progressive enhancement. I don't want to add a scroll event, which has the potential to slow down the browser.
@ehsann_95 I see you're still putting in a bit of effort on the other branch. You're welcome to continue to do so, but there's still quite of work to be done. You're welcome to review the new MR, which should be a bit closer to complete.
- 🇺🇸United States Glottus
So far, so good! I've opened another issue that has come up now, but I don't know if it is related to these specific changes. The good news is that by using the adjustments to static variables as in your MR, I have been able to move forward without re-experiencing the resources issues that had been plaguing this project after enabling eca_vbo.
The new issue is here: https://www.drupal.org/project/eca_vbo/issues/3464059 🐛 Edits to model appear to break configuration in View Active
- 🇩🇪Germany Anybody Porta Westfalica
@W01F would be great if you could provide more details!
- 🇩🇪Germany Anybody Porta Westfalica
Yeah this should work out of the box and totally make sense. I'd rate this as bug, could we start with tests showing that it doesn't work as expected yet and improve the issue summary to tell what's expected and the actual state?
Or is this simply a duplicate of 🐛 Still not working with certain letters Needs work for e.g. Japanese characters?
Curiously this error seems to have resolved itself. The error above was reported during a cache rebuild, but is now gone after the cache rebuild and http restart.
- 🇩🇪Germany Anybody Porta Westfalica
Thank you @sanduhrs, great work! Could someone please add a test for these cases?
Do we need other tests due to the modifier addition?https://www.php.net/manual/en/reference.pcre.pattern.modifiers.php
- 🇩🇪Germany Anybody Porta Westfalica
@draenen could you please prepare a MR from the patch? And maybe it would be worth to link this issue in the comment?
Are there better alternatives to this approach, maybe?
This should be fixed in the 3.x version.
Getting WSOD with Unsupported operand types in LinkFormatter->buildUrl() error in D10.1 after updating to PHP 8.1.27.
Successfully installed the patch in #22, but still getting an error.PHP Fatal error: Uncaught TypeError: array_merge(): Argument #1 must be of type array, string given in /var/www/.../web/core/modules/link/src/Plugin/Field/FieldFormatter/LinkFormatter.php:247 Stack trace: #0 /var/www/.../web/core/modules/link/src/Plugin/Field/FieldFormatter/LinkFormatter.php(247): array_merge() #1 /var/www/.../web/core/modules/link/src/Plugin/Field/FieldFormatter/LinkFormatter.php(178): Drupal\link\Plugin\Field\FieldFormatter\LinkFormatter->buildUrl() #2 /var/www/.../web/core/lib/Drupal/Core/Field/FormatterBase.php(89): Drupal\link\Plugin\Field\FieldFormatter\LinkFormatter->viewElements() #3 /var/www/.../web/core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php(265): Drupal\Core\Field\FormatterBase->view() #4 /var/www/.../web/core/modules/layout_builder/src/Entity/LayoutBuilderEntityViewDisplay.php(268): Drupal\Core\Entity\Entity\EntityViewDisplay->buildMultiple() #5 /var/www/.../web/core/lib/Drupal/Core/Entity/EntityViewBuilder.php(339): Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay->buildMultiple() #6 /var/www/.../web/core/modules/node/src/NodeViewBuilder.php(24): Drupal\Core\Entity\EntityViewBuilder->buildComponents() #7 /var/www/.../web/core/lib/Drupal/Core/Entity/EntityViewBuilder.php(281): Drupal\node\NodeViewBuilder->buildComponents() #8 /var/www/.../web/core/lib/Drupal/Core/Entity/EntityViewBuilder.php(238): Drupal\Core\Entity\EntityViewBuilder->buildMultiple() #9 [internal function]: Drupal\Core\Entity\EntityViewBuilder->build() #10 /var/www/.../web/core/lib/Drupal/Core/Security/DoTrustedCallbackTrait.php(111): call_user_func_array() #11 /var/www/.../web/core/lib/Drupal/Core/Render/Renderer.php(788): Drupal\Core\Render\Renderer->doTrustedCallback() #12 /var/www/.../web/core/lib/Drupal/Core/Render/Renderer.php(377): Drupal\Core\Render\Renderer->doCallback() #13 /var/www/.../web/core/lib/Drupal/Core/Render/Renderer.php(204): Drupal\Core\Render\Renderer->doRender() #14 /var/www/.../web/core/lib/Drupal/Core/Render/Renderer.php(160): Drupal\Core\Render\Renderer->render() #15 /var/www/.../web/core/lib/Drupal/Core/Render/Renderer.php(583): Drupal\Core\Render\Renderer->Drupal\Core\Render\{closure}() #16 /var/www/.../web/core/lib/Drupal/Core/Render/Renderer.php(161): Drupal\Core\Render\Renderer->executeInRenderContext() #17 /var/www/.../web/core/modules/node/src/Plugin/Search/NodeSearch.php(525): Drupal\Core\Render\Renderer->renderPlain() #18 /var/www/.../web/core/modules/node/src/Plugin/Search/NodeSearch.php(489): Drupal\node\Plugin\Search\NodeSearch->indexNode() #19 /var/www/.../web/core/modules/search/search.module(80): Drupal\node\Plugin\Search\NodeSearch->updateIndex() #20 /var/www/.../web/core/lib/Drupal/Core/Cron.php(335): search_cron() #21 /var/www/.../web/core/lib/Drupal/Core/Extension/ModuleHandler.php(388): Drupal\Core\Cron->Drupal\Core\{closure}() #22 /var/www/.../web/core/lib/Drupal/Core/Cron.php(343): Drupal\Core\Extension\ModuleHandler->invokeAllWith() #23 /var/www/.../web/core/lib/Drupal/Core/Cron.php(159): Drupal\Core\Cron->invokeCronHandlers() #24 /var/www/.../web/core/lib/Drupal/Core/ProxyClass/Cron.php(75): Drupal\Core\Cron->run() #25 /var/www/.../web/core/modules/automated_cron/src/EventSubscriber/AutomatedCron.php(65): Drupal\Core\ProxyClass\Cron->run() #26 [internal function]: Drupal\automated_cron\EventSubscriber\AutomatedCron->onTerminate() #27 /var/www/.../web/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php(111): call_user_func() #28 /var/www/.../vendor/symfony/http-kernel/HttpKernel.php(115): Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch() #29 /var/www/.../web/core/lib/Drupal/Core/StackMiddleware/StackedHttpKernel.php(63): Symfony\Component\HttpKernel\HttpKernel->terminate() #30 /var/www/.../web/core/lib/Drupal/Core/DrupalKernel.php(688): Drupal\Core\StackMiddleware\StackedHttpKernel->terminate() #31 /var/www/.../vendor/drush/drush/src/Boot/DrupalBoot8.php(331): Drupal\Core\DrupalKernel->terminate() #32 [internal function]: Drush\Boot\DrupalBoot8->terminate() #33 {main} thrown in /var/www/.../web/core/modules/link/src/Plugin/Field/FieldFormatter/LinkFormatter.php on line 247
- 🇺🇸United States jrb Raleigh-Durham Area, NC, USA
Here's a patch from the MR.
Note that this patch is for 11.x while the patch in #2 above will apply to 10.x.
- 🇺🇸United States smustgrave
Lets add some test coverage for this change please.
- @jrb opened merge request.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
Actually, the
Component
config entity already handles this, largely. So we don't need all that infrastructure.But we're missing some validation here.
- @prem-suthar opened merge request.
- 🇮🇳India Prem Suthar gujrat
Prem Suthar → made their first commit to this issue’s fork.
- 🇧🇪Belgium herved
This seems to relate/overlap with 🐛 Error: Call to a member function uuid() on null in Drupal\media_library\MediaLibraryEditorOpener->getSelectionResponse() (line 72 of /app/docroot/core/modules/media_library/src/MediaLibraryEditorOpener.php) Needs work .
The MR there seems to fix it but I haven't compared the code of both solutions yet. I still think that one #3458369: DDEV support for Cypress tests is in then it would be easier for me to resolve the test failures.
- 🇵🇹Portugal joao.ramos
Hi @svenseeberg,
Thanks for following up on my incomplete information.
To whom it may concern, I'm actually using the patch from #138, with a local patch on top of it adding the request session. As after @idebr very useful patch there was already a trajectory on the issue and I haven't had time to specifically analyze its impact, although it works on my project, I haven't yet added a patch here. I'll do that soon.Cheers!
- 🇩🇪Germany jurgenhaas Gottmadingen
I think I got it: the variable to collect the plugin definitions needs to be static to catch all the scenarios that can lead to the infinite loop. Please review and test if that fixes your scenario as well.
- @jurgenhaas opened merge request.
- 🇳🇿New Zealand quietone New Zealand
And no longer a child of the core issue.
@flyke, thank you very much anyway. Even it didn‘t help in my (strange) case, it surely will help others :)
best regards
- 🇧🇪Belgium flyke
Vincent_Jo: I listed the entity types that my workaround code works for, taxonomy terms / vocabularies is none of them. Working entity types for my demo code are 'node', 'media', 'commerce_product'. Of course you can add more types in the swich statement inside the query() function if you add the correct db field that contains the id. For everyone else: my apologies for polluting this issue queue. I do think that this custom filter can help others (only as a workaround) who come here looking for a solution until this issue gets resolved.
- @imustakim opened merge request.
- 🇮🇳India samit.310@gmail.com
samit.310@gmail.com → changed the visibility of the branch 3450175-typeerror-fieldtypepluginmanagercreatefielditem-called to hidden.
The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
Hi flyke, thanks for the response ... none of the operators worked. Maybe it‘s on the custom image field in this vocabulary? ... oh, just realized, when adding a custom description field it showed up in all four languages, so I get four duplicated items with each description field in another language. Even though I have a translation language filter (Content language selected for page) active. So this could be the issue here in my case.
well ....
- @samit310gmailcom opened merge request.
- 🇮🇳India samit.310@gmail.com
samit.310@gmail.com → made their first commit to this issue’s fork.
- 🇧🇪Belgium flyke
Hi Vincent_Jo, just adding it should be enough, no need to set anything (leave default and just save). Maybe someone can improve my code to clear the settings for that filter.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
Clearly, @Utkarsh_33 has already started writing tests prior to 📌 DDEV support for Cypress tests Needs work being ready, so removing the prefix 🚀
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
@Utkarsh_33 FYI: see what @hooroomoo wrote in #21, which I explained in some more detail at https://git.drupalcode.org/project/experience_builder/-/merge_requests/9... 😊
- 🇺🇸United States douggreen Winchester, VA
I still get a PHP warning in AttributeArray (line 79) because the original input array is statically cached in ViewExecutable::getExposedInput() on line 726, which happens before the data is validated. We need to validate the data before it is saved.
- 🇳🇿New Zealand quietone New Zealand
Trying for a better title. On scanning I kept thinking this was a regression.
- 🇺🇸United States mherchel Gainesville, FL, US
I created a new branch using SDC and CSS only (using scroll animations).
Right now the scroll stuff only works on Chromium. The main functionality works on all browsers.
That being said, I think we might still want to use Intersection Observer so that the show/hide works also works on all browsers.
- @mherchel opened merge request.
- 🇦🇺Australia acbramley
Now that I'm actually manually testing the scenario described in the IS I don't think this is a valid bug anymore (see my comment on the MR).
Perhaps at the time this issue was created, we didn't call
$this->fail()
insidecheckPermissions
OR as per #32 phpunit halts execution on a fail.Since we fail when there's an invalid permission, the original issue in the IS does not happen because drupalLogin is never called when an invalid permission is passed through.
Settintg to PMNMI to see if there's other scenarios that we need to account for.
- 🇺🇸United States Glottus
I'm not sure this is completely fixed.
Building out a new D10 site (currently 10.3.1) where we use Webform as a per-entity registration form and ECA to handle various automated tasks, we got to a stage where we want to start building models that can get triggered in bulk when viewing a the webform submissions attached to a node.
This site is still very much in early stages with fewer than 10 such event nodes and/or webform submissions to list, and there are only 6 or 7 ECA models built; just one single model and one single View which implement the VBO functionality so far.
After adding and enabling the vbo and eca_vbo modules (2.0.0), we immediately began noticing resource issues, mainly "allowed memory size... exhausted". This is true on both local development (DDEV) and test server environments where we tried increasing settings such as "memory_limit" in our .user.ini file, but with not much improvement. Occasionally pages will fail with a white screen and short message to help, and finally one of the messages appeared with more detail:
ECA ran into error from third party in the context of "Collecting all available actions": Allowed memory size of 536870912 bytes exhausted (tried to allocate 126976 bytes) Line 113 of /var/www/html/web/core/lib/Drupal/Core/Database/StatementWrapperIterator.php
To attempt some basic troubleshooting, I modified the /eca_vbo/src/Plugin/Action/Derivative/VboExecuteDeriver.php file to wrap one of the foreach loops with an artificial limit:
$i = 100; while ($i > 0) { foreach (($eca->get('events') ?? []) as $event) { if (!in_array($event['plugin'], $plugin_ids, TRUE)) { continue; } $operation_name = $event['configuration']['operation_name'] ?? ''; $action_id = strtolower(preg_replace("/[^a-zA-Z0-9]+/", "_", trim($operation_name))); if ($action_id !== '' && $action_id !== '_') { if (!isset($this->definitions[$action_id])) { $this->definitions[$action_id] = [ 'label' => str_replace('_', ' ', ucfirst($operation_name)), 'operation_name' => $operation_name, ] + $base_plugin_definition; } $this->definitions[$action_id]['eca_config'][] = $eca->id(); } } $i--; }
Not knowing exactly how this is SUPPOSED to exit cleanly, I wanted to see if functionality returned, and it did. Without the artificial limit above, I quickly get errors that begin like the following trace:
The website encountered an unexpected error. Try again later.
Error: Xdebug has detected a possible infinite loop, and aborted your script with a stack depth of '1000' frames in Drupal\Component\DependencyInjection\Container->hasParameter() (line 342 of core/lib/Drupal/Component/DependencyInjection/Container.php).
Drupal\Component\DependencyInjection\Container->get('string_translation') (Line: 197)
Drupal::service('string_translation') (Line: 215)
Drupal\Core\StringTranslation\TranslatableMarkup->getStringTranslation() (Line: 190)
Drupal\Core\StringTranslation\TranslatableMarkup->render() (Line: 15)
Drupal\Core\StringTranslation\TranslatableMarkup->__toString()
sprintf('%s %s', Object, Object) (Line: 70)
Drupal\Core\Action\Plugin\Action\Derivative\EntityActionDeriverBase->getDerivativeDefinitions(Array) (Line: 101)
Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator->getDerivatives(Array) (Line: 87)
Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator->getDefinitions() (Line: 337)
Drupal\Core\Plugin\DefaultPluginManager->findDefinitions() (Line: 213)
Drupal\Core\Plugin\DefaultPluginManager->getDefinitions() (Line: 22)
Drupal\Core\Plugin\DefaultPluginManager->getDefinition('entity:delete_action:media') (Line: 16)
Drupal\Core\Plugin\Factory\ContainerFactory->createInstance('entity:delete_action:media', Array) (Line: 83)
Drupal\Component\Plugin\PluginManagerBase->createInstance('entity:delete_action:media', Array) (Line: 178)
Drupal\eca\PluginManager\Action->createInstance('entity:delete_action:media', Array) (Line: 62)
Drupal\Core\Plugin\DefaultSingleLazyPluginCollection->initializePlugin('entity:delete_action:media') (Line: 80)
Drupal\Component\Plugin\LazyPluginCollection->get('entity:delete_action:media') (Line: 18)
Drupal\Core\Action\ActionPluginCollection->get('entity:delete_action:media') (Line: 88)
Drupal\Core\Plugin\DefaultSingleLazyPluginCollection->setConfiguration(Array) (Line: 104)
Drupal\Core\Plugin\DefaultSingleLazyPluginCollection->addInstanceId('entity:delete_action:media', Array) (Line: 55)
Drupal\Core\Plugin\DefaultSingleLazyPluginCollection->__construct(Object, 'entity:delete_action:media', Array) (Line: 114)
Drupal\system\Entity\Action->getPluginCollection() (Line: 130)
Drupal\system\Entity\Action->getPlugin() (Line: 145)
Drupal\system\Entity\Action->getPluginDefinition() (Line: 72)
Drupal\eca\Plugin\Action\PreConfiguredActionDeriver->getDerivativeDefinitions(Array) (Line: 101)
Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator->getDerivatives(Array) (Line: 87)
Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator->getDefinitions() (Line: 337)
Drupal\Core\Plugin\DefaultPluginManager->findDefinitions() (Line: 213)
Drupal\Core\Plugin\DefaultPluginManager->getDefinitions() (Line: 22)
Drupal\Core\Plugin\DefaultPluginManager->getDefinition('action_message_action') (Line: 16)
Drupal\Core\Plugin\Factory\ContainerFactory->createInstance('action_message_action', Array) (Line: 83)
Drupal\Component\Plugin\PluginManagerBase->createInstance('action_message_action', Array) (Line: 62)
Drupal\Core\Plugin\DefaultSingleLazyPluginCollection->initializePlugin('action_message_action') (Line: 80)
Drupal\Component\Plugin\LazyPluginCollection->get('action_message_action') (Line: 88)
Drupal\Core\Plugin\DefaultSingleLazyPluginCollection->setConfiguration(Array) (Line: 104)
Drupal\Core\Plugin\DefaultSingleLazyPluginCollection->addInstanceId('action_message_action', Array) (Line: 55)
Drupal\Core\Plugin\DefaultSingleLazyPluginCollection->__construct(Object, 'action_message_action', Array) (Line: 871)
Drupal\eca\Entity\Eca->getPluginCollections() (Line: 165)
Drupal\Core\Config\Entity\ConfigEntityBase->set('weight', 0) (Line: 163)
Drupal\eca\Entity\Eca::postLoad(Object, Array) (Line: 389)
Drupal\Core\Entity\EntityStorageBase->postLoad(Array) (Line: 319)
Drupal\Core\Entity\EntityStorageBase->loadMultiple() (Line: 56) The Needs Review Queue Bot → tested this issue.
While you are making the above changes, we recommend that you convert this patch to a merge request → . Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)
- @hooroomoo opened merge request.
- 🇺🇸United States jrb Raleigh-Durham Area, NC, USA
Here is a patch that adds a counter and uses that to make sure that
uniqueBundleId
is always unique.This fixes our issue with Flag and should fix the other referenced issues above.
- 🇫🇷France zenimagine
Nice work. For the animation, how will you handle the infinite scrolling pages? Maybe by disabling the feature?
- 🇺🇸United States mherchel Gainesville, FL, US
Left some general comments. We should also move this to a single directory component. It's a perfect use case.
- 🇺🇦Ukraine vlad.dancer Kyiv
FYI: there is no Photoswipe Dynamic Plugin field formatter settings inside views field settings (when you select show fields and use Photoswipe as field formatter). For that you need to apply this patch 🐛 Formatter third party settings missing from Views UI Needs work
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
Just discussed #3461499-17: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings → with @laurii, he agrees that all defaults should be specified in the SDC, not in a
Component
. See #3461499-19: Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings → and preceding comments.That means this is obsolete.
I tested patch #8 and it's working. I can confirm that the user is no longer redirected to the preview link when accessing the content view page logged in!
- 🇩🇪Germany Anybody Porta Westfalica
The fork needs to be updated and conflicts needs to be resolved. Anyway, this looks quite far implemented. Thank you @tBKoT for pushing things forward.
✨ Implement a generic revision UI Fixed and 🐛 Revert and Delete actions should not be available for the current revision Fixed are both resolved in core. So this should be compared to what https://www.drupal.org/node/3160443 → suggests.Maybe "Needs tests" tag can be removed, I can see the Mr has tests.
I think there are quite a few projects where it would be great to have revisions for products and product variations to be able to track changes and revert them if needed!
- 🇮🇳India amanmansuri72
I have made some necessary changes, Kindly review MR 8926.
Thanks
- 🇺🇸United States hooroomoo
This should be rebased again now that 📌 Implement component states Fixed which this MR was originally built on top of is in HEAD :)
- 🇺🇸United States smustgrave
Still appears to need test coverage
Also can a proposed solution be added to the issue summary please
Thanks!
- 🇧🇬Bulgaria pfrenssen Sofia
Yeah this is not a bug, but a configuration error on the user's behalf. I hit this on a CI environment because the translations folder was missing.
I guess we can improve the error message.
- @amanmansuri72 opened merge request.
- First commit to issue fork.
- ivnish Poland
I rerolled issue fork to 11.x
I also use this patch in my project and it fixed bug from issue title
- 🇩🇪Germany svenseeberg
I commented 2 lines in the
src/Controller/FacetBlockAjaxController.php
and now I do at least not get an error message:<?php foreach ($facets_blocks as $block_id => $block_selector) { $block_entity = $this->storage->load($block_id); if ($block_entity) { // Render a block, then add it to the response as a replace command. $block_view = $this->entityTypeManager ->getViewBuilder('block') ->view($block_entity); //$block_view = (string) $this->renderer->renderPlain($block_view); //$response->addCommand(new ReplaceCommand($block_selector, $block_view)); } } ?>
The request to /de/facets-block-ajax?_wrapper_format=drupal_ajax then returns a valid looking response with 200 OK. However, I'm not sure if the GUI is working as intended.
- @ivnish opened merge request.
- First commit to issue fork.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
Precise steps to reproduce are missing. What was the precise filename of the image that caused the validation error?
Better yet: use that exact filename to add new test coverage to
\Drupal\Tests\experience_builder\Kernel\PropSourceTest
, to trigger the exact error message you were getting! 😄 - 🇮🇹Italy plach Venezia
+++ b/core/modules/system/src/Plugin/ImageToolkit/GDToolkit.php @@ -281,6 +281,16 @@ protected function load() { return FALSE; ... + $this->logger->error("The image toolkit '@toolkit' failed loading image '@image'. Failed function: @function.", [ + '@toolkit' => $this->getPluginId(), + '@image' => $this->getSource(), + '@function' => $function, + ]); + $this->preLoadInfo = NULL; + return FALSE;
Instead of duplicating this logic, why don't we just set
$image
toFALSE
in thecatch
branch? Hi, thanks @flyke, I tried it out immediately. I‘m getting the filter in the filter criteria. It asks me to choose an operator.
Unfortunately none of my selections work to eliminate the duplicates.
Any further advice?regards
... having high hope for this :)
- 🇳🇴Norway eiriksm Norway
Hey 👋 Thanks for your contribution! 🚀
The issue summary is a bit lacking. Especially with regards to how to reproduce the issue. Could you for example try to formulate it in a way where we could reproduce it from a clean install?
And what versions are you using? Drupal and PHP especially.
In addition, it's a bit disappointing that our tests do not cover this, so I would like to add test coverage as well. This would also be much easier with exact steps to reproduce
Thanks again ♥️
- @magaki opened merge request.
- 🇮🇳India Chandansha
I've tested MR 8872 with the help of View live preview link.
i attached the video for refference.
I move it to RTBC.
THANKS!! - First commit to issue fork.
- 🇧🇪Belgium flyke
Typically when I try out a workaround for this issue, I create a custom 'Remove duplicate results' filter like this:
mymodule/mymodule.module:
function mymodule_views_data() { $data['views']['remove_duplicates'] = [ 'title' => t('Remove duplicate results'), 'filter' => [ 'title' => t('Remove duplicate results'), 'field' => 'id', 'id' => 'remove_duplicates', 'click sortable' => FALSE, ], ]; return $data; }
mymodule/src/Plugin/views/filter/RemoveDuplicatesFilter.php:
<?php namespace Drupal\mymodule\Plugin\views\filter; use Drupal\Core\Form\FormStateInterface; use Drupal\views\Plugin\views\filter\StringFilter; /** * Remove duplicates by adding id field and group by for that field. * * @ingroup views_filter_handlers * * @ViewsFilter("remove_duplicates") */ class RemoveDuplicatesFilter extends StringFilter { /** * {@inheritdoc} */ protected function defineOptions() { $options = parent::defineOptions(); return $options; } /** * {@inheritdoc} */ public function buildOptionsForm(&$form, FormStateInterface $form_state) { parent::buildOptionsForm($form, $form_state); } /** * {@inheritdoc} */ public function query() { // Set distinct to TRUE; $this->query->distinct = TRUE; $this->query->options['distinct'] = TRUE; /** * @var string contains the entity type the view uses. */ $entity_type = $this->view->getBaseEntityType()->id(); switch ($entity_type) { case 'node': $entity_id_label = 'nid'; break; case 'media': $entity_id_label = 'id'; break; case 'commerce_product': $entity_id_label = 'product_id'; break; default: break; } // Add identifier field and group by it. if (!empty($entity_type) && !empty($entity_id_label)) { $this->query->addField($entity_type, $entity_id_label, '', ['function' => 'groupby']); $this->query->addGroupBy($entity_id_label); } } }
You need to clear cache first after adding this code, and then add this filter to your view.
As you can see in the code, this only works for views for node, media and commerce_product entities, but it served its function on several projects here. - 🇬🇧United Kingdom catch
@pifagor in what kind of context is the link being rendered? This sounds like a render caching issue.
CSRF tokens are rendered via a placeholder/lazy builder
(see
$placeholder_render_array = [ '#lazy_builder' => ['route_processor_csrf:renderPlaceholderCsrfToken', [$path]], ];
from RouteProcessorCsrf
However, if the placeholder rendering itself gets cached somehow, or is used in a different context like e-mail, then you end up with invalid CSRF links.
Re #38 : I don't think that issue solved the problem with the pager here and it's neither addressing the issue of sorting which is addressed here.
- @s_leu opened merge request.
- First commit to issue fork.
- 🇺🇸United States bnjmnm Ann Arbor, MI
- The contextual form seems to be loading an older version of the form - I'm guessing the MR moved some stuff around which would up preserving some code that has since been refactored
- Clicking a component to make it active/selected takes us to /xb/component/ which is great, but refreshing a page on that URL results in a 404. While I'm very OK with incremental progress, refreshing the UI page is something I do frequently while developing for it and I'd like it to at least redirect back to /ui
- 🇺🇸United States euk
The patch in #71 🐛 BaseFieldOverride fails to take into account ContentEntityInterface::bundleFieldDefinitions() when invoking onFieldDefinitionUpdate() Needs work still fails. This time with the following error:
Error: Call to a member function getSettings() on null in /var/www/web/core/lib/Drupal/Core/Field/FieldConfigBase.php on line 376 #0 /var/www/web/core/lib/Drupal/Core/Field/FieldConfigBase.php(289): Drupal\Core\Field\FieldConfigBase->getSettings()
This happens on config import, and seems to be related to translations somehow (probably not the cause but one of the triggers for this issue), as the only update affecting the current build was enabling translation...
Automatically closed - issue fixed for 2 weeks with no activity.
- 🇺🇸United States bradjones1 Digital Nomad Life
@Akhil Babu, thanks for this! I think this change could be made pretty easily, and your MR gets at the core (only?) real code change that needs to be made. (I think a LogicException might be more appropriate here?)
What this needs, though, is tests that validate the universe of allowed and disallowed patterns. That would also help in documenting in code what is and isn't allowed by Drupal's route builder.
- Issue created by @godotislate
- 🇪🇸Spain marcoscano Barcelona, Spain
@silvi.addweb thanks for trying to help, but opening a MR with the same changes of the patch, when in the previous comment I mentioned what was missing in the patch, isn't enough to move this to RTBC
- 🇪🇸Spain marcoscano Barcelona, Spain
Apologies for not looking sooner into this. We'll need test coverage for the new feature we are introducing.
- 🇺🇸United States smustgrave
Was previously tagged for tests which are still needed
Thanks.
- 🇦🇺Australia VladimirAus Brisbane, Australia
hook_update
for new config property?
- 🇦🇺Australia VladimirAus Brisbane, Australia
VladimirAus → changed the visibility of the branch 3313343-disable-cron to hidden.
- 🇮🇳India Akhil Babu Chengannur
This would be very helpfull. Right now, Drupal just shows a 'page not found' message when the route is invalid.
- @akhil-babu opened merge request.
- 🇮🇳India Akhil Babu Chengannur
Akhil Babu → made their first commit to this issue’s fork.
- 🇬🇧United Kingdom scott_euser
Fine if someone wants to contribute this, otherwise going to leave closed. Work should be done to the 2x branch if so
-
scott_euser →
committed 2b41d9c6 on 2.0.x authored by
DieterHolvoet →
Issue #3067325 by scott_euser, DieterHolvoet, Anybody: Provide the whole...
-
scott_euser →
committed 2b41d9c6 on 2.0.x authored by
DieterHolvoet →
- 🇺🇸United States cilefen
I am moving this to the development branch in case there is a merge request.
Can you author a failing test for this?
- @anupsingh-0 opened merge request.
- 🇮🇳India anup.singh
anup.singh → changed the visibility of the branch 3463518-php-warning- to hidden.
- 🇩🇰Denmark ressa Copenhagen
Thanks for fixing the version @quietone. Sorry that I keep selecting the wrong version. I will try to get it right next time.
Adding possibly related issue.
- Issue created by @ressa
- 🇮🇳India anup.singh
anup.singh → changed the visibility of the branch 3463518-php-warning- to active.
- 🇮🇳India anup.singh
anup.singh → changed the visibility of the branch 3463518-php-warning- to active.
- 🇮🇳India anup.singh
anup.singh → changed the visibility of the branch 3463518-php-warning- to hidden.
- 🇬🇧United Kingdom mjpa
This patch will need work as when applied, the Webform UI stops showing any webforms in the list.
Hi, it doesn‘t work for me on 10.1. (I can‘t update core because of an domain access issue).
My views-page has contextual filter: "Has tax term ID with depth" and "Has tax term ID with depth modifier".
Then I add a relationship "Taxonomy term on node". Just then I have the field "Tax-Image" available in the fields section.
Adding this getting duplicates!
Query-settings don‘t work. Actually hiding the image from display if any of the two options are activated.I follow this thread since my first attempt with D8. Now I‘m still experiencing this issue on D10. It‘s crucial to get this fixed on my taxonomy driven job-board site migrated from D7.
regards
- 🇮🇳India samit.310@gmail.com
Hi @handkerchief,
Agree with your point, but here the idea is the only trusted or some specific user roles have this permission Use the site in maintenance mode(
access site in maintenance mode
).The functionality will remain the same for other users, meaning whenever the maintenance mode will on all the users except those with the above permission will logout.
Thanks
Samit K. - 🇺🇦Ukraine pifagor 🇺🇦 Rivne
The custom code just preparing the link for logout.
"Have you have called \Drupal\Core\Routing\UrlGeneratorInterface::generateFromRoute with user.logout" - yes, but as I wrote before, the token is regenerated constantly, and despite using generateFromRoute, it is constantly out of date in csrfToken->validate. - Issue created by @bradjones1
- 🇫🇷France zenimagine
hi, I found the website below which has a button to go up to the top of the page. It's great because when you scroll the page, it displays around the arrow the length of the page. Can you add an animation for the length of the page (a circle like on the website below):
- First commit to issue fork.
- 🇺🇸United States Danny Englander San Diego
I ran into the same issue with a reference of multiple taxonomy terms. #117 working for me with 10.2.5. Pager works fine as well as exposed filters. Thank you!
- 🇺🇸United States erutan
I'm not using ECA for anything involving anonymous users, but good to know about that issue.
I've disabled the module myself and am just going without comments on views until things clear up.
- 🇺🇸United States smustgrave
Seems all the tags are still needed. Also forgot issue summary update from before
- 🇺🇸United States smustgrave
Only briefly skimmed but any change will need test coverage.
Also issue summary appears incomplete, recommend using default issue template. Sounds like from previous comment MR solution isn't along the idea so definitely needs a summary update.
- 🇺🇸United States smustgrave
Thanks for working on this
Tagging for issue summary as that appears to be incomplete, recommend using the standard issue template
Tagging for tests as feel a simple assertion could be added to an existing test.
Also, the settings form in 2.x is broken.
🐛 Linkchecker settings form is broken Active
- 🇮🇹Italy kopeboy Mainland
I see no mention of CSS issue in the summary nor in the patch, which does the job! Thank you! +3
- @solideogloria opened merge request.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
(To elaborate on #4: @hooroomoo landed that in ✨ Create UI scaffolding for the primary panel (left sidebar) Needs work .)
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
solideogloria → changed the visibility of the branch 2.0.x to hidden.
- 🇬🇧United Kingdom jessebaker
Assigning to @hooroomoo as they have been working on the left hand menu so far.
Sorry hooroomoo, it looks like there are some iterations/updates required to the menu work you did now that the designs have progressed further.
- 🇬🇧United Kingdom jessebaker
@Wim Leers
Yes this can be continued. I recommend that when @hooroomoo is back tomorrow that they and @Utkarsh_33 sync to ensure there is a clear decision as to who continues this work to push it over the finish line.
Once #3458369 📌 DDEV support for Cypress tests Needs work is complete and tests are easier to write I think some tests to ensure the following should be implemented
- Ensure that after selected a component at the root level, a button is shown that says "Add section". Clicking that button should open the menu to the Section Templates part of the left menu.
- Ensure that after selected a component below the root level (e.g. a component inside a slot), a button is shown that says "Add component". Clicking that button should open the menu to the Components part of the left menu.
- 🇬🇧United Kingdom jessebaker
Current Status:
Due to my ongoing challenges in getting Cypress tests working locally, there is still a requirement for tests on this work.
Tests that still need to be written are:
- [e2e] A test that visits the XB, selects a components and checks that the URL is /xb/component/
- [e2e] A test that visits the XB, selects one component and then another and then uses the back button in the browser and is able to navigate back through the two selected components and back to /xb.
- [e2e] A test that visits the XB and opens the component sidebar (to the correct component) via the
element in the TopBar.tsx
The first test ensures that setting the selectedComponent state updates the URL.
The 2nd ensures that the browser history states are working correctly.
The 3rd ensures that navigating via the URL updates the state. - 🇮🇳India Kanchan Bhogade
Hi
I've tested MR 8872 on Drupal 11
The MR is applied cleanly...For no page scroll, there is No back-to-top button as expected, and when the page scrolls, the back-to-top button appears.
Attaching SS for reference
RTBC+1 - 🇬🇧United Kingdom catch
The filter is only for text. It shouldn't be used for HTML.
The output of a text format is by definition HTML. Even if you're entering markdown or something, the end result is HTML, and the auto line break filter can theoretically be applied at any point in that process. You could say it should only be applied at the beginning, before any other HTML formatting is applied from different filters, and that editors shouldn't copy and paste paragraph breaks when using a format with the automatic line break filter, but the text format system doesn't enforce this at all, and I've had plenty of sites where a mixture of handwritten and copy and pasted content gets put into the same text format.
The question is, does this filter make any sense at all in conjunction with CKEditor fields
The media filter can be used without ckeditor5, so the line break filter ought to be able to work with it - and comments above suggest this is broken without any involvement from ckeditor5 at all.
There might well be a case for adding some validation to prevent the auto-line break filter from being configured alongside ckeditor5 though but that feels like a new issue to me.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
@jessebaker: Is it okay for @Utkarsh_33 to continue here?
@erutan, I tried it with the above mentioned patches as per your recommendation.
Unfortunately, it doesn't work unless I remove the message subscribe example module.I still need to refresh the page.
Before Drupal 10.3 they worked together without any problems.There is an ECA issue with having ajax comments enabled for anonymous users when viewing a node.That's why I had to remove the ajax comment module again.
https://www.drupal.org/project/eca/issues/3462785 🐛 TypeError: eca content entity view mode alter Active- 🇩🇪Germany rgpublic Düsseldorf 🇩🇪 🇪🇺
* IMHO, I think this is correct. The filter is only for text. It shouldn't be used for HTML. I don't see any valid use-case either. Whether it really needs to check and silently fail or whether a description would enough is anyone's guess. There are other filters which just use the issue description to specifiy their restrictions in order to work correctly ("only use after/before this and that"). They don't check and silently fail in these cases either.
* This issue morphed into sth. else though. The original issue as still seen in the issue description was "we can't guarantee that the rendered representation of that media item will be a single HTML element". That's still a valid concern and by far isn't solved by just disabling this filter. But perhaps a new issue could be filed for this. The far more complicated problem to solve is: If a media *entity(!)* is rendered it unsurprisingly consists of entity *fields(!)*. When rendering these fields, there are DIV tags introduced at the very least because the default field item template contains DIV tags. I know this affects issues with far reaching consequences and this is probably not easy to change, but my take on this is: This is actually wrong. A DIV cannot appear inside a SPAN tag. This causes invalid HTML. Whenever a field is rendered inline you will get these problems which is kind-of sad. It could perhaps be solved without turning the whole of Drupal on its head if we said that field elements should be rendered with a special SPAN tag template just for the media entity display mode or sth. along these lines. - 🇬🇧United Kingdom catch
Note there's now a stable trash module in contrib https://www.drupal.org/project/trash →
I haven't personally used it, but I believe it is the result of @amateescu's comment in #50.
- 🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
I don't — I have zero experience with Open API 😅
But a quick search in @larowlan's
decoupled_lb_api
module reveals https://git.drupalcode.org/project/decoupled_lb_api/-/blob/1.x/tests/src..., whoseassertDataCompliesWithApiSpecification()
achieves exactly that :) - 🇦🇺Australia VladimirAus Brisbane, Australia
Thanks for your contribution.
Can we update the it to the latest branch? - 🇩🇪Germany drubb Sindelfingen
The question is, does this filter make any sense at all in conjunction with CKEditor fields? I think it's primarily meant for use with plain text fields, as CKEditor does this conversion already. I can't think of a use case for fields powered by CKE, so maybe we need a hint not to use it, or prevent usage at all for CKE fields. At least it should fail silently and not destroy the markup given by CKE.
- 🇦🇺Australia acbramley
This was triaged as part of Bug Smash Initiative.
I tried reproducing this on 10.3 but couldn't:
\Drupal::entityTypeManager()->getStorage('media_type') ->getQuery() ->condition('name', ['Document'], 'IN') ->accessCheck(FALSE)->execute();
I'm tempted to close this due to the lack of comments and updates since the last time this was traiged, but we will leave it in PMNMI.
Cheers.
- 🇬🇧United Kingdom catch
Also if we were to add this, I think it should be its own filter doing this generally for all site-relative links, not within the file reference filter.
For example https://www.drupal.org/project/rel_to_abs → provides this in contrib.
- 🇬🇧United Kingdom catch
The issue summary doesn't explain the problem with relative URLs, why is this a problem?
Additionally, Drupal sites can be run from multiple domains, using domain module and similar. Relative URLs allow this to work, I don't think we're able to cache text formats by arbitrary cache contexts so it would probably break that.
- 🇳🇿New Zealand quietone New Zealand
The patch here needs to be converted to an MR and allow the tests to run. I am all for tests but what is the test to do here? Is it just to confirm the refresh is done once? That needs clarification.
- 🇺🇸United States erutan
@pearls there's some other needs review patches here that seem to help with containing the scope / not having conflicts.
I'm currently running the following in my composer.json and haven't noticed any issues with related modules. That said it's of minimal use when trying to have comments in a view all it does is stop the redirect to the entity edit page on save (which is great).
"drupal/ajax_comments": { "fix ajax on non default views": "https://www.drupal.org/files/issues/2024-03-18/ajax_comments-ajax_non_default_view_mode-2896916-beta5-81.patch", "remove deprecated library": "https://www.drupal.org/files/issues/2023-01-20/ajax_comments-jquery-form-3335188-2.patch", "fix non-unique IDs": "https://www.drupal.org/files/issues/2021-04-09/3208008-2.patch" },
- 🇺🇸United States DaleTrexel Minnesota, USA
Updating the version to 3.0.x so that the issue fork is off the correct branch.
We're actually using 2.0.x, but this should apply to both, and we may be able to solve more than one issue with this, so keeping relevance by focusing on the highest version.
Also simplifying/shortening the title.
- 🇺🇸United States smustgrave
Can someone test if 📌 Add 'Is empty (NULL)' and 'Is not empty (NOT NULL)' operators to boolean field filtering Needs work solves the issue here? If not what should be addressed here that isn't addressed other there.
- 🇫🇷France mably
We are being hit by this rather annoying bug when using the views_auto_refresh → module.
Looks like a simple fix, it's surprising that it hasn't been merged yet.
Thanks for the patch!
- 🇺🇸United States pfrilling Minster, OH
This has been resolved and the API Spec test should be passing now.
> # 1 The new test is failing:Do you have any examples for testing the fixtures against the spec? If not, I'll start researching how to make that happen.
- 🇨🇭Switzerland handkerchief
@samit.310@gmail.com But not logging out does not automatically mean that users should continue to use the website despite maintenance.
Example from practice:
Website is briefly switched to maintenance mode several times a day because automatic backups are performed. However, it is correct that during backups, no user administers the website and therefore should not have the permission “Use the site in maintenance mode”. But the users should not all be logged out just because maintenance mode was switched on briefly. If you have a website with a community where many users are logged in, then they are automatically logged out several times a day. - 🇮🇳India samit.310@gmail.com
HI,
We have permission provided by Drupal Use the site in maintenance mode(
access site in maintenance mode
) that allow any user to work on maintenance mode, A simple change can fix this user logout issue,Here all the users who have Use the site in maintenance mode(
access site in maintenance mode
) permission, will not logout if the site goes to maintenance mode.Here is the PR link: https://git.drupalcode.org/project/drupal/-/merge_requests/8876/diffs
Thanks
Samit K. - @samit310gmailcom opened merge request.
- 🇮🇳India samit.310@gmail.com
samit.310@gmail.com → made their first commit to this issue’s fork.
- 🇺🇸United States skowyra Boston
We've also been running into this behavior; html tags and classes get stripped out in CKEditor 5. I can see where normalization could be the culprit, but in our case, we have a clunky work-around when the behavior occurs. If resaving doesn't work after several attempts, we copy the content (Source), paste into a text editor, add the new class or html there, copy the updated content back. That usually works.
The fact that we can eventually save it indicates that normalization would not be the root cause. Let me add, this behavior occurs in nodes and webforms, plus we use Site Studio where it occurs in our components.
This started happening when we upgraded to CKEditor 5. We're currently on Drupal 10.2.4, but will be going up to 10.3 very soon.
- First commit to issue fork.
- 🇮🇳India Prashant.c Dharamshala
@junaidpv
Do you mean executing a batch process in a custom/contrib form using
#ajax
with an AJAX submit handler? - 🇸🇰Slovakia kaszarobert
Needs a reroll. Also patch #11 and #12 are completely different and many things were removed from it in #12. @EthanT can you explain that? Otherwise #11 seems to be the right one with the previous propositions to make a form configurable setting to enable/disable to schedule transitions for past dates.
- @ahsannazir opened merge request.
- 🇫🇮Finland lauriii Finland
The "Add section" button should be only active when the component is active / selected, meaning that it should not be visible when hovering over a component that is not currently active / selected. Currently the button appears in the hover state, even when the component is not active.
Now the button is more inline with the design.Attaching the screenshots for the same.
- 🇺🇸United States phenaproxima Massachusetts
Glad we got this isolated, I think with that in mind it shouldn't be too hard to write a test case.
- @huzooka opened merge request.
- 🇬🇧United Kingdom catch
Moving this to filter module. Should be possible to write a test case given the examples above to reproduce the duplicate link, then we can try to fix it in _filter_autop
- @kksandr opened merge request.
- Issue created by @kay64
- 🇺🇸United States DamienMcKenna NH, USA
The MR is empty, and the patch needs a slight improvement for coding standards. It might also be worth using !is_numeric() rather than just != to make the intent more clear. Lastly, I think there should be test coverage for this change.
#81 didn't work.
By the way, Ajax comments module breaks other modules working like eca, message subscription etc. after Drupal 10.3
When I uninstall ajax comments these modules work fine. I'm not sure what the main problem is.- 🇩🇪Germany drubb Sindelfingen
Did a quick test to look at this filter in isolation, by calling the _filter_autop() function directly. This is broken indeed:
$output = _filter_autop('<p><a href="https://google.de"><drupal-media/></a></p>')
Result:
<p><a href="https://google.de"></p>\n <drupal-media/></a></p>
- 🇦🇹Austria drunken monkey Vienna, Austria
Thanks a lot for your input, that was very helpful. I see that there are cases in which no individual (scalar) property can give you the needed information for indexing a field, where you do need complex data to arrive at a single, scalar field value. It would indeed be great if we could support this use case. From what I can see, at least the contract of
\Drupal\search_api\DataType\DataTypeInterface::getValue()
doesn’t actually forbid us from passing complex values (arrays or objects) as$value
– but the assumption was definitely built into all of the implementations, at least of the default types, and might also be present in other places in our code.In any case, instead of returning
$data
directly, what about just returning$data->getValue()
? That would seem a bit more benign. It also would enable us to better handle cases where the property contains multiple values – though, on the other hand, it seems like that should already be handled by the$definition->isList()
check at the top of the method.
Also, I guess your approach has the big advantage of carrying type information with it – if you just get an associative array like['value' => 2, 'options' => 'abc']
it could potentially come from any number of data types, while your Search API data type plugin probably just handles a specific one. (At this point it would be helpful if we’d allow Search API data type plugins to restrict the types of properties they can be used for, but that’s another issue entirely.)I created a draft MR with my suggestion, let’s see whether this blows up with our existing tests. In any case, we’ll need further tests to make sure this really enables use cases such as yours, and that it doesn’t cause error when using the built-in data types with complex properties.
Please let me know what you think. I’m now sceptic myself regarding the switch from$data
to$data->getValue()
, so we can also go with your approach if you agree. - @drunken-monkey opened merge request.