- 🇯🇵Japan ptmkenny
If it “needs tests” it cannot be RTBC. The code is committed with the tests if it needs tests.
- 🇯🇴Jordan Ahmad Khader
Using 🐛 Allow altering of the language fallback for path aliases Needs review and the commit https://git.drupalcode.org/project/language_hierarchy/-/commit/c3731dfd0..., I can confirm that URL aliases function properly on Drupal 10.x. Thanks for your awesome work, James Williams.
- 🇺🇸United States smustgrave
Got bit by this nightmare again over in better_exposed_filters.
Cleaning up the tags but still leaving in NW for the tests.
- 🇪🇸Spain omarlopesino
I've tested this in several projects and it works fine, moving to RTBC. The code is also fine.
- 🇺🇸United States smustgrave
Didn't check the Unit failure to see if related but definitely something that will need test coverage.
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 fieldfield_brand
.
In my understanding, the first timeextractFields
is called, theComplexDataInterface $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, insteadextractField
is called. I extracted your logic in a helper function and used it inextractField
as well... and here I had some thoughts.@drunken monkey I refrained from injecting the
$index
variable to theextractField
function, since it already hasFieldInterface $field
as an argument, and we should be able to extract the index from there. I had to pass the index to theextractFieldValues
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 inextractFieldValues
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 theextractFieldValues
function, to avoid possible side-effects.- 🇮🇳India dev16.addweb
Re-roll the patch #20 according to the latest code changes.
- 🇵🇰Pakistan isalmanhaider
+1
@grimreaper, I support the proposed resolution to parse version information from Composer when not available in .info.yml.This change will improve consistency and prevent errors on the status report page, ensuring that version information is reliably available. Leveraging Composer for this purpose aligns with best practices in dependency management and enhances the overall reliability of the system.
- @utkarsh_33 opened merge request.
- @joelpittet opened merge request.
Closing as duplicate of https://www.drupal.org/project/drupal/issues/3360837 🐛 Users with permissions for toolbar but not administration tray get "Uncaught TypeError: $orientationToggleButton[0] is undefined" in console Needs work
- 🇬🇧United Kingdom scott_euser
Looks like the issue summary needs an update? Drupal 11 with default Olivero theme, no changes from clean install other than turning on twig.config debug to true now results in this which appears correct:
The selected customisation comes from olivero.theme:
function olivero_theme_suggestions_menu_alter(&$suggestions, array $variables) { if (isset($variables['attributes']['region'])) { $suggestions[] = 'menu__' . $variables['attributes']['region']; } }
For the region 'primary_menu'. Is there a step I am missing to reproduce?
- 🇺🇸United States nicxvan
Can you please add those changes to the merge request? People can then generate local patches as needed and the core team uses the actual mr for testing and merging.
- @grimreaper opened merge request.
- Issue created by @Grimreaper
- last update
1 day ago Custom Commands Failed - 🇺🇸United States sapetm
Hey, wanted to add a tweak on patch from comment #53. The elseif on line 191 of /core/lib/Drupal/Core/Entity/Query/Sql/Tables.php didn't have a condition and was throwing warnings occasionally. Here is that patch with a condition for the elseif. This patch applies and works for us in Drupal 10.2.6 and PHP 8.1.27.
- 🇫🇷France 5
Maybe we can refactor this cancel confirm route so it asks your password one last time.
Something like changing the link form cancellation_url to /user/login?destination=cancellation_url, which should transparently
- if not connected, makes the user land on user login form instead of unattended 403 (making it understand it may login in order to confirm the cancellation of its account)
- if connected, makes the user land to the cancellation confirmation page
?
- last update
1 day ago Patch Failed to Apply - 🇺🇸United States sapetm
Sorry folks, third time's the charm, or something like that. I uploaded the wrong patch file with my other two comments. This is the patch I am using for Drupal 10.2.6 and PHP 8.1.27. It is identical to the patch in comment #102 but with the addition of an array_unique() in the second hunk when setting the value of the $suggestions array in the render function. This fixes the issue for us. Hope it helps!
- 🇩🇪Germany Knurg
Thanks for working on that. Unfortunately you were right, I forgot one more spot where the tracking deletion was called.
However I seem not to be able to push to the merge request and I don't know how to push on it otherwise :/
I thought the negation would be more convenient because it does not touch the existing behaviour and I don't have to change any defaults in the database etc. I would really be more happy with an interface for the users to simply check the option. Our typical target group is art historians who hardly are able to install Drupal at all and can't really handle the installation itself - so having no option here does simply give our project team even more load in infrastructural parts, while it could be avoided :/
I've attached a patch for IndexLoadItemsTest.php.
- 🇺🇸United States smustgrave
Believe this will need sub-maintainer approval for such a change, but could this functionality also live in a contrib module?
If approved will need an upgrade path and test coverage.
- 🇺🇸United States smustgrave
Question is this reproducible in just core and only with the contrib?
Believe we will need a test case to show the issue.
- 🇫🇷France Grimreaper France 🇫🇷
Hi,
I tried to convert patch from comment 117 into a MR to more easily update it.
After trying to figure out in more details what it is was doing, I think I will open a separated issue for #3440128: version in info.yml for general project → .
In my case, I only want to try to extract versions from Composer if not present in .info.yml to provide site admin versions in admin pages.
- 🇺🇦Ukraine _tarik_ Lutsk
I have added simple code that will move additional action buttons before the actions array are deleted from the form.
FYI: The changes are compatible with the Drupal 10.2.5
- @_tarik_ opened merge request.
- 🇧🇪Belgium DieterHolvoet Brussels
I added a submit handler that empties the state fields if the date fields are empty. I think all that's left to do now are tests.
- last update
2 days ago Unable to generate test groups - Issue created by @_tarik_
- 🇺🇸United States bnjmnm Ann Arbor, MI
If we're running into an infinite recursion issue, I recommend first finding where the recursion is occurring. This
We already know that there is an infinite recursion problem when
#type
is set in the render array.This is most likely due to a condition that executes code only when
#type
is present. The two most likely ways PHP would check for the presence of#type
areisset($foo['#type'])
or!empty($foo['#type'])
We can use a regular expression to find places where these conditions might be set
[^\!]isset\(.*\['#type'\]\)
Will look for an "isset" call without a preceding exclamation point, followed by an array path that ends#type
. The regex could be even more precise, but just this narrows things down to only 9 places this could be happening.
Most of the results can be ruled out because they are in modules/themes that don't need to be running for this error to occur, they are in tests, or it belongs to a class for a feature that is unrelated to the issue.Expand the results and you can rule out additional possibilities:
This leaves only 5 possible places, making it pretty easy to check each with an xdebug breakpoint, and see which condition is getting hit infinite times.
If I place the breakpoint in the Renderer.php condition, it appears to loop infinitely when it hits an element with a
form_element_label
#theme.Without even having to know that much about the render system, just a little bit of PHP / regex / xdebug we can see the recursion is happening in this block in Renderer.php
// If the default values for this element have not been loaded yet, populate // them. if (isset($elements['#type']) && empty($elements['#defaults_loaded'])) { $elements += $this->elementInfo->getInfo($elements['#type']); }
Is this best accomplished by changing the condition, or by changing what
this->elementInfo->getInfo($elements['#type']);
returns, or something entirely different? - 🇩🇪Germany osopolar 🇩🇪 GER 🌐
For me it still seems to be relevant. I just edited the default content view (under /admin/structure/views/view/content) to reproduce the issue. There I added an new field with html in the label. The html of the label gets escaped. Switching to a different display style (i.e. unformatted list) has the same result.
- First commit to issue fork.
- 🇩🇪Germany webflo
I have committed 🐛 InvalidArgumentException: The timestamp must be numeric. in Drupal\Component\Datetime\DateTimePlus::createFromTimestamp() (line 201 of /var/www/html/www/core/lib/Drupal/Component/Datetime/DateTimePlus.php). Active (including a test). I think its the same issue.
- @grimreaper opened merge request.
- 🇦🇺Australia gordon Melbourne
This problem seems to be related to #2738051 🐛 \Drupal\views\Plugin\views\query\Sql::getCacheTags and getCacheMaxAge don't take into account that some entities can be NULL Needs work , but in my case the other didn't fix my issue but caused another issue, and this one fixed it. They both seem to be a very similar issue and solved in slightly different methods.
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Thanks for the reroll
We need some tests for this and also would need a rebuild task that used batch to recreate after re-enable - 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
larowlan → changed the visibility of the branch 512864-comment-count-query to hidden.
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Thanks, we need some tests for this new behaviour though.
- 🇨🇭Switzerland Berdir Switzerland
I also just had a case in a project where the current behavior was unexpected. #20 wouldn't work for me, because it wasn't a direct link, but part of the trait. In my case, the deeper menu link wasn't actually want I wanted, it was the one higher up that was in a completely different section of the site. The deeper one was actually a cross-reference from a different section of the website.
So what would work for me is just removing the array_reverse(), which basically makes the behavior consistent with core. But that seems rather arbitrary either way. In my case, the deeper link also had extra query arguments. The logic in #19 and rerolls also seems rather arbitrary, which might or might not be what you expect.
There's ✨ Allow other modules to alter the active link trail Needs work as well, if you want to apply extra rules on matching. For this to be committed in a BC way, the behavior would probably need to be configurable and especially if it gets as complex as latest patches, requires pretty extensive test coverage to cover all those extra checks.
- 🇺🇸United States jnicola
@alexpott I believe the linked Project Browser work will need it as it needs a way to display all the available local recipes.
We also developed a recipe_ui module at Drupalcon that will also need to discover recipes. The intention was to put it in core, but since the Project Browser push is the main focus, it would be a contrib module... if we even push out what we built at all.
In general though being able to discover recipes seems like a useful chunk of functionality for folks so as everyone isn't reinventing the wheel.
- @kksandr opened merge request.
- Issue created by @kay64
- 🇨🇭Switzerland Berdir Switzerland
I think it would make more sense if that's implemented in the frontend *before* saving, otherwise you have to go back after you already saved and edit it again. Similar to the you have unsaved changes message after reordering.
That said, opinionated things are tricky to do in core. first, it should be in menu_ui, because node module should not depend on menu link stuff, then it needs to handle cases when the node title field isn't displayed and so on.
- last update
3 days ago 29,689 pass - 🇩🇪Germany mbuechner Frankfurt
Please ignore the previous patch. I have attached the wrong file. :-(
- 🇩🇪Germany mbuechner Frankfurt
I can confirm this. Here is a (quick) patch for the problem.
- 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺
Created new PR with the computed property approach.
Needs tests, but ready for discussion. - 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺
penyaskito → changed the visibility of the branch 3447203-improve-dx-of to hidden.
- @penyaskito opened merge request.
- 🇬🇧United Kingdom alexpott 🇪🇺🌍
I've opened 📌 Move RecipeDiscovery into RecipeConfigurator Needs review to remove the existing RecipeDiscovery. I'm not sure we need this work yet in core but at least there will less confusion about the purpose of the class that was called RecipeDiscovery.
- 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺
@larowlan suggested on slack:
"
So I think a computed property on list item base makes sense
Like we have processed on text item and date on date item
"I get this as him not buying my initial implementation (which I neither was very happy with), but buying the idea. I'll be working in that ASAP.
- 🇺🇸United States PapaGrande
I'm getting sporadic reports from a customer of this error on a hosted site with a webform using a file upload field, but can't reproduce the error myself, either locally or in production. Can others provide information about:
- Do the errors happen in all browsers?
- Are you logged in or not? Do the errors happen for anonymous users?
- What AJAX settings are enabled in the webform?
- Is the site local or hosted? If hosted, which provider?
- Is JavaScript aggregation enabled?
- Is a CDN in front of the site? If so, which one and are there special caching rules in place?
- @penyaskito opened merge request.
- Issue created by @penyaskito
- Open on Drupal.org →Core: 10.2.1 + Environment: PHP 8.1 & MySQL 5.7last update
4 days ago Waiting for branch to pass - 🇺🇸United States froboy Chicago, IL
It's been a minute. Here's a rerolled patch for 8.x-2.12. Interdiff shows only indexes and surrounding text have changed.
- 🇺🇸United States bnjmnm Ann Arbor, MI
Perhaps the solution can be more targeted and the sticky recalculated on the details toggle event instead of the current approach that uses the more-often-occuring
scroll
event.It's also worth pointing out that this will likely no longer be an issue once this issue lands: 📌 Make drupal.tableheader only use CSS for sticky table headers Needs review
However, that would only benefit 11.x, and it would be nice to get this bug addressed for Drupal 10 so this could still be worth doing.
- last update
4 days ago Patch Failed to Apply - last update
4 days ago Patch Failed to Apply - last update
4 days ago Patch Failed to Apply - 🇺🇸United States bnjmnm Ann Arbor, MI
There's an error occurring in the logs as well, the "Content Type: example" recipe does not have a 'name' property, so
logo: $this->getRecipeLogo($path, $recipe['name'])
in the plugin is hitting an error with that recipe. - 🇺🇸United States bnjmnm Ann Arbor, MI
We need some tests for this - among other things we need to be able to prevent changes that reintroduce "this is a module" assumptions. The issue that preceded this, 🐛 "View Commands" button assumes project is a DO module (and that a project has commands) Fixed , probably should have had tests but we chose to wait until this issue so the tests were based on an actual plugin (faster to write and more representative of IRL scenarios!)
- 🇨🇦Canada bisonbleu
For the short term, simply adding an ID to the checkbox label does the job; the use case is Simplenews subscriptions on user register page.
/** * Implements template_preprocess_form_element_label(). */ function my_custom_theme_preprocess_form_element_label(&$variables) { $id = $variables['element']['#id'] ?? ''; if ($id && str_contains($id, 'edit-subscriptions-')) { $variables['attributes']['id'] = 'edit-subscriptions--description'; } }
- 🇳🇱Netherlands ekes
Added Kernel tests to describe the functionality of the patch in https://git.drupalcode.org/issue/date_recur-3010184/-/compare/3.5.x...3.... The branch does not (yet) look at the views integration. I'll be honest I'm still a bit confused that it indexes the default version, and the latest version; and then only the default and latest. I guess this is where looking at the views integration comes in.
Additional explanatory note https://git.drupalcode.org/issue/date_recur-3010184/-/compare/3.5.x...3.... The existing tests passed with field values of cardinality 2, but storage of 1, because they were working on the inserted entity. But on loading (the default revision) from storage, of course, the second value was not loaded as it was not stored. Basically presently should you save an entity with a date recur field with more values than its cardinality it does calculate occurrences for the invalid deltas.
- First commit to issue fork.
- 🇺🇸United States generalredneck
I'm pretty sure this is related to 🐛 Notice: Undefined index: value in Drupal\views\Plugin\views\filter\NumericFilter->acceptExposedInput() Needs work . It may possible be a duplicate? though I landed here because I"m getting a similar issue even after upgrading to 10.2.5 where the prior fix was released.
-
darvanen →
committed 0d2dd283 on 2.x
Issue #3274693 by darvanen: Write tests for webform validation handler
-
darvanen →
committed 0d2dd283 on 2.x
- @darvanen opened merge request.
- 🇩🇪Germany lmoeni
I tested the MR today and it works fine when I use the Images button (except the things discussed in the comments starting from #87).
I cannot get it working with the media library though. Is there anything that I need to keep in mind while testing it with the media library? - 🇺🇸United States bradjones1 Digital Nomad Life
bradjones1 → changed the visibility of the branch 3252278-jsonapi-computed-field-item-list-cache-metadata to active.
- 🇺🇸United States bradjones1 Digital Nomad Life
Rebased my MR, no regressions there, this still needs specific test coverage.
- 🇩🇪Germany jan.stoeckler
@bradjones1
Thanks for the MR, however I'm curious how you handle the empty case? It seems to require special handling, unless I'm missing something obvious here.
Can you elaborate on why empty field item lists need special handling, i seem to be missing or not fully grasping something on that front?
- 🇺🇸United States bradjones1 Digital Nomad Life
bradjones1 → changed the visibility of the branch 11.x to hidden.
- 🇺🇸United States bradjones1 Digital Nomad Life
bradjones1 → changed the visibility of the branch 3252278-jsonapi-computed-field-item-list-cache-metadata to hidden.
- 🇺🇸United States bradjones1 Digital Nomad Life
bradjones1 → changed the visibility of the branch 3252278-jsonapi--computed to hidden.
- @bradjones1 opened merge request.
- First commit to issue fork.
- 🇨🇦Canada bisonbleu
Just running into this issue in a custom theme. Here is the code sample that generates the «Broken ARIA reference» error:
<!-- BEGIN OUTPUT from 'core/modules/system/templates/input.html.twig' --> <input data-drupal-selector="edit-subscriptions-sip-n-savour" aria-describedby="edit-subscriptions--description" type="checkbox" id="edit-subscriptions-sip-n-savour" name="subscriptions[sip_n_savour]" value="sip_n_savour" class="form-checkbox"> <!-- END OUTPUT from 'core/modules/system/templates/input.html.twig' --> <!-- BEGIN OUTPUT from 'core/modules/system/templates/form-element-label.html.twig' --> <label for="edit-subscriptions-sip-n-savour" class="option">SIP n’ SAVOUR</label>
- 🇧🇴Bolivia alvarito75 Cochabamba
Confirming #141 patch works for
- Drupal 10.2.6
- PHP 8.2.12
- 🇺🇸United States bburg Washington D.C.
I was testing out this module/patch. I seem to get this error, and it fails to generate the image:
Deprecated function: strtolower(): Passing null to parameter #1 ($string) of type string is deprecated in Drupal\avif\Controller\ImageStyleDownloadController->deliver() (line 79 of modules/contrib/avif/src/Controller/ImageStyleDownloadController.php).
- last update
5 days ago Patch Failed to Apply - 🇺🇸United States sapetm
I seem to have messed up the naming convention for the patch files. This is my first time uploading a patch file. Sorry about that! Hope I did it right this time. Below is from my previous post.
This is a tweak on the patch from comment #102. We have had this problem for a while and we've always had to, in the render function at the last point where the suggestions array is defined, apply an array_unique() to it. This has always fixed the problem for us, but I haven't tested thoroughly to see if this in fact fixes it everywhere. We are running Drupal 10.2.6 and PHP 8.1.27. Hope this helps!
- last update
5 days ago Patch Failed to Apply - last update
5 days ago Patch Failed to Apply - 🇺🇸United States westsonoma
I thought I'd searched exhaustively for a Drupal module that authenticates to Gmail using OAuth2, but I guess not. Yesterday I came across Gmail API ( https://www.drupal.org/project/gmail → ). I installed it, and it seems to work fine; no need to write another.
- 🇺🇸United States sapetm
This is a tweak on the patch from comment #102. We have had this problem for a while and we've always had to, in the render function at the last point where the suggestions array is defined, apply an array_unique() to it. This has always fixed the problem for us, but I haven't tested thoroughly to see if this in fact fixes it everywhere. We are running Drupal 10.2.6 and PHP 8.1.27. Hope this helps!
- 🇵🇱Poland gugalamaciek
@sakthi_dev I'll try to produce 11.x version in free minute... but can't promise when exactly.
- 🇵🇱Poland gugalamaciek
Ups... I forgot about schema updates. So this is #49 + schema fixes.
Automatically closed - issue fixed for 2 weeks with no activity.
- 🇺🇸United States bnjmnm Ann Arbor, MI
We are making changes in claro templates only.Do we need to do the same of default theme and then expand tests or shall we create a follow-up issue for default theme?
\Drupal\form_test\Form\FormTestDetailsForm
already tests the default theme. it is simplest to add a few lines to that test to run the test with the default theme and then with Claro. Use a@dataProvider
Set up your dataProvider and put this at the beginning of the test to set the theme when
$theme
isn't false, and that does everything needed without having to add a new test class that is 99% the same as an existing one.if ($theme) { $this->config('system.theme') ->set('default', $theme) ->save(); }
- 🇨🇦Canada Liam Morland Ontario, CA 🇨🇦
Opened a merge request with the patch in #42.
- @liam-morland opened merge request.
- 🇳🇱Netherlands pahles
I tried updating the module today but this patch no longer gets applied. Composer just hangs. It works when I revert to 8.x-1.1 but cannot be applied in 8x-1.2 or 8x-1.3.
- 🇺🇸United States richgerdes New Jersey, USA
So we can add functionality to EntityInterface if we want to.
While we can add new apis, it's better to not add BC breaks if we can avoid it. Similarly to the entity class, we are adding a function to the
EntityStorageInterface
too, which is also a BC break. We probably want to add an interface that add the create function. It would be nice to see these two new interfaces be removed in a future version of Drupal core (probably not 11 at this point, but 12 maybe?).Is the pre/postDuplicate stuff necessary?
Sure, technically a hook is all that's needed to cover the basic description, but when I ran into this issue, i also wanted to check the duplicate state at save time, which isn't covered by just using a hook unless I create those properties myself during the hook.
Both the config entity base and content entity base make use of the pre/post hooks. We could move that logic into the storage class, but I would recommend against it. If the api is setting an expectation for what will happen with an entity, then the logic should live on the entity class, even it its more functions. IMO the storage service should deal directly with loading entities, and not with managing entity properties field data. This is the best way to ensure that data is updated correctly over time.
Moving this logic to the entity storage class is a bad idea because a developer could override the storage class with a custom implementation. While we hope that the class override extends the existing base storage, we can't be sure that it will and that means the new class might not include the same logic for updating the entity during these events and for the duplicate operation to work consistently, then entity class is the best place for the logic.
The other question is what does core already do? The entity classes already have
pre/postSave()
andpre/postDelete()
functions, so there is precedence to have a similar api structure for duplicate. Additionally, since the duplicate process is similar to the create process, which would call the entity's constructor, we should have a way for the entity class to react to that as well, no?On the flip side, there is a lot of logic in the existing content/config entity storage base classes as well. For example both
createRevision()
andcreateTranslation()
exist on theContentEntityStorageBase
. Given the possibility of overriding the class the field value updates should probably be moved to the entity class instead, unless there is a strong need to tie the logic into the storage service such as loading the new serial id from the database. Given the current logic for these two functions, the main reason we need to use the storage handler for entity operations is to expose the module handler service in order to invoke info and alter hooks. If we want hooks, this may be unavoidable, but the hooks could be run along side.Given the above, I would still couple any entity data changes as closely to the entity class as possible, and for translation and revision support, that should likely live in the relevant traits instead of the generalized storage.
I am for simplifying logic where possible. Right now during clone the following happens.
- User calls
EntityBase::create()
- That gods to
EntityStorageBase->create()
- We then call
EntityBase::preCreate()
- Then
EntityStorageBase->doCreate()
which callsEntityBase::__construct()
EntityStorageBase->create()
then calls$entity->enforceIsNew()
- Folloed by
$entity->post create() ?</li> <li>Finally the "entity_create" hook</li> </ol> This flow is pretty lean, and doesn't expose any pre/post operations on the storage class itself, so either we would need need to add support for pre/post functions there, or a developer would be forced to override <?php EntityStorageBase->create()
to alter the logic, which seems more messy then adding a few extra functions to the process ahead of time. Maybe its not necessary, but seems like its a better Developer experience to provide the apis up front, which is what I think this patch is currently doing to mimic the create flow.
What is the use case for the getDuplicateSource() and related methods?
I think the duplicate source is a valid thing to keep track of during the duplication process. Sure this could just be a variable passed to the classes, but I think it makes sense to store it on the entity level as a temporary value, in order for use of the "duplicate" state outside of the hooks. For example would be nice if a module wanted to change something in the form during load, based on the duplicate state, there is no way for it to identify that without the class property/function.
What's left?
Skimming the comments, it seems like the last major issue outside of the above discussion is that "there is also no new test coverage for the hooks yet". Is there anything else that still needs to be done here?
- User calls
Hiding problematic patches, since #42 is still what people are using.
- 🇺🇸United States moshe weitzman Boston, MA
Do folks think this could cause pages to show the same tabs to one node? This is happenning on mass.gov and on several other sites. See https://www.drupal.org/forum/support/module-development-and-code-questio... →
assert() is non-functional in Prod so I dont think that approach is as helpful.
- 🇺🇸United States smustgrave
Thank you for reporting
Will need to update the MR for 11.x as the latest development branch
Will also need a failing test case that shows the issue. - 🇮🇳India sakthi_dev
@gugalamaciek, could you please share the diff. I have created an MR for 11.x 2ith #49.
- @sakthi_dev opened merge request.
- 🇵🇱Poland gugalamaciek
#50 is #49 extended with End date optional #153 ( https://www.drupal.org/project/drupal/issues/2794481#comment-14839868 ✨ Allow end date to be optional Needs work )
- @veronicaseveryn opened merge request.
- First commit to issue fork.
Test for this by expanding \Drupal\form_test\Form\FormTestDetailsForm to try with Claro in addition to the default test theme, and assert the expected class is present.
We are making changes in claro templates only.Do we need to do the same of default theme and then expand tests or shall we create a follow-up issue for default theme?
- 🇵🇱Poland gugalamaciek
Improved patch which covers:
- adds all_day column in DB for datetime range fields. Set it as disabled for all existing data with datetime type
- allow to enable all day dates for datetime type & set default value for it in field settings
- widget to set all day, making sure that date is stored in UTC when all day is selected (in general timezone is ignored when all day event is set)
- GraphQL - share allDay property for datetime range fields
- formatter - for now only supported inn default formatter. You can specify date format for all day and non all day dates
- js - when you'll select all day, time is set to 00:00:00 for start & end dates - @utkarsh_33 opened merge request.
- First commit to issue fork.
- 🇦🇹Austria drunken monkey Vienna, Austria
Thanks for reporting this issue and already providing a very nice patch!
Since we now switched to using MRs instead of patches, I created an MR with your patch. (I now see that we still had the old advice to not use MRs in the new issue notice, so my fault.)Some remarks on the patch itself:
- First off, I prefer to avoid negations in setting names wherever possible, so I changed the setting name to
delete_on_fail
, defaulting toTRUE
. - Second, your description doesn’t square completely with what your patch does. Are you sure it works correctly on your site? From your description, it would seem like the problem is not failed indexing, but failed loading of items. In this case, yes, Search API thinks the item has been deleted without us noticing it, so also deletes it from the index on the search server. I changed the patch so that the setting now controls that behavior, not the other one during indexing. (As far as I can see, the code you changed only affects items deliberately removed from indexing, e.g., by the “Entity status” processor.)
Please tell me if that was the solution you were looking for, or whether indeed your old patch did what you wanted it to. In the latter case, I think I’d need a more detailed description of the problem and what exactly happens. - Architecturally, the fact that a dysfunctional data store cannot be distinguished from missing entities is unfortunate. I’d really like the datasource to throw an exception in this case, instead of just returning an empty array. Unfortunately, this is currently not even allowed by the contract of
\Drupal\search_api\Datasource\DatasourceInterface::loadMultiple()
, so really just my own fault. Then I guess this new setting is probably really the best solution (even though it might mean that some actually deleted items stay in the index on your site). - This is a small enough change to not need much approval, I think, so I’ll just merge this after hearing back from you and waiting for a week or so for any additional feedback. However, I’d guess this will not be a massively popular option. Therefore, and because it’s really only something for advanced users, I think I’m removing it from the index config form. You, and any others in need of it, will have to manually enable it via a config export/import. I hope that is acceptable, but I really don’t want to push yet more options on already overwhelmed new users.
- Finally, this will need automated test coverage before it can be merged. Would you be able to provide that? There is already a test for the affected functionality in
\Drupal\Tests\search_api\Kernel\Index\IndexLoadItemsTest::testMissingItems()
, we’d just need to add a copy of that method that verifies that the deletions do not happen when the new setting is disabled. (Doing it via a@dataProvider
would also work, not sure what makes more sense.)
- First off, I prefer to avoid negations in setting names wherever possible, so I changed the setting name to
- @drunken-monkey opened merge request.
- 🇦🇹Austria drunken monkey Vienna, Austria
drunken monkey → made their first commit to this issue’s fork.
- 🇺🇸United States generalredneck
I imported the view used in the automated test. I don't think this patch fixes the full problem. I used a modified version of the test's content and put in the title of
I'm testing special characters day&night© ® € ™ ← ↑ → ↓ ♠ ♣ ♥ ♦
. It really is just the Ampersand (&) that causes issues. While the "title" is fixed in the view, the other fields that rendered are not.Here is the full source code of the view outputed by the test. Note that the link, description, dc:creator, and guid> fields are all double encoding the ampersand.
<?xml version="1.0" encoding="utf-8"?> <rss xmlns:dc="http://purl.org/dc/elements/1.1/" version="2.0" xml:base="http://drupal-10.lndo.site/"> <channel> <title>Rss</title> <link>http://drupal-10.lndo.site/</link> <description/> <language>en</language> <item> <title>Hey Hey</title> <link>http://drupal-10.lndo.site/Hey%20%3Csup%3EHey%3C/sup%3E</link> <description>Hey <sup>Hey</sup></description> <pubDate>Hey Hey</pubDate> <dc:creator>Hey <sup>Hey</sup></dc:creator> <guid isPermaLink="true">http://drupal-10.lndo.site/Hey%20%3Csup%3EHey%3C/sup%3E</guid> </item> <item> <title>I'm testing special characters day&night© ® € ™ ← ↑ → ↓ ♠ ♣ ♥ ♦</title> <link>http://drupal-10.lndo.site/I%26#039;m testing special characters day&amp;night© ® € ™ ← ↑ → ↓ ♠ ♣ ♥ ♦</link> <description>I'm testing special characters day&amp;night© ® € ™ ← ↑ → ↓ ♠ ♣ ♥ ♦</description> <pubDate>I&#039;m testing special characters day&amp;night© ® € ™ ← ↑ → ↓ ♠ ♣ ♥ ♦</pubDate> <dc:creator>I'm testing special characters day&night© ® € ™ ← ↑ → ↓ ♠ ♣ ♥ ♦</dc:creator> <guid isPermaLink="true">http://drupal-10.lndo.site/I%26#039;m testing special characters day&amp;night© ® € ™ ← ↑ → ↓ ♠ ♣ ♥ ♦</guid> </item> </channel> </rss>
- 🇬🇧United Kingdom joehuggans Harrogate, UK
Here's the patch from #41 re-rolled working with Drupal v 10.2.6.
- last update
7 days ago Build Successful - 🇯🇵Japan ptmkenny
And another problem: My testing was all through Feeds Tamper, where this MR helps out.
But if you map a field and do not tamper it, even with this MR, Feeds will expect lowercase, not uppercase.
TL;DR: This is going to be a hard problem to fix, so for now, I suggest putting a clear message on the project page: Uppercase characters are not currently supported in JSON keys (for example, { "uppercaseDoesNotWorkHere": "You can use uppercase here" }).