Tested this on a site. It was causing serious performance problems on a node with ~20k revisions. The slow queries that where showing up didn't make sense either b/c they should have been the optimized case.
After discussing with Alex on slack, and confirming the poor performance went away when the database was copied to a different server(by way of a database dump) we decided this was likely an issue with the storage causing it to choose a bad subquery logic.
After hours I ran the following. A warning for those that follow, after hours was a good instinct. The queries ran immediately but then took down the site as all other queries touching those tables where queued until it finished running background tasks on the table.
ANALYZE TABLE node_field_data PERSISTENT FOR ALL;
ANALYZE TABLE node_revision PERSISTENT FOR ALL;
This seems to have fixed the problem and the slow node that was timing out now loads immediately and the query the slow query returned instantly.
That 401 is interesting. The createInternalField method is trying to log that 401 and it shouldn't be triggering an exception so its weird that's causing a problem. All the service wrapper classes try to avoid throwing exceptions and always returns a response.
GuzzleHttp\Exception\RequestException: cURL error 60: SSL certificate problem: unable to get local issuer certificate
This is more interesting. I suspect all the API wrappers aren't able to catch or log this and don't document it as a possibility. I might to look a bit deeper at how we communicate since this is likely a problem for all requests.
Yeah, I'm currently using key with config overrides but its kind of a pain. Deeper support is on my radar but I haven't gotten to it. MR welcome.
If I understand you're creating 2 components, 1 for your main search content and another for a standalone search block you can embed elsewhere on the page? I had planned on at some point adding another block type or something for enabling a standalone search block but expected there might be some complexities involved so hadn't gotten to it yet. If this is working for you that's a clever solution.
Sounds like you're using it correctly. Its supposed to be as simple as adding the block which the exception of secured searches being really confusing to explain.
Accepted the documentation and fixes in those issues so I think this is resolved unless you want to provide some additional help hooks or help pages to document this as well.
I see, organization is required but SourceID isn't required on the organization. That's sort of by design since you might be using the organization only for atomic or something and not pushing Documentation looks good and I'll look at improving the UI to filter the select list or provide some warnings about invalid states.
If you want to look into what's triggering that third party dependency or maybe narrow down a site module that could trigger it or even give a list of modules that would help narrow down what's causing this.
Sorry, I missed you report. There's some quirky things going on with config but this is very odd. Also this exactly how we deploy these so wouldn't have expected it to break like this.
>If I manually edit the exported config to remove the content dependency, this error does not occur.
Hm... this sounds like a really good clue but just clicking around I wasn't able to recreate getting that dependency added and the config entity for components only adds a relationship to the referenced organization.
Looking at ConfigEntityBase the only thing I can think of is there's a third party module adding a dependency but I don't have any idea what that would be.
You're getting to the root of the problem and why the previous issue was open for so long and why this is so difficult to fix.
Really there's two problems
1) its painfully generalized for all entity types
2) The assumption of content moderation/workflow/etc processes in core for that the "active" revision is the latest revision and mix and match that concept in their interfaces. And then contrib that builds on top of these inherits that complexity as Klausi pointed out #64.
Really 1 shouldn't matter because I'm pretty sure we're all in agreement we shouldn't even be using it as "latest" doesn't really have any useful meaning.
And 2... Well unraveling that is an entire community project which is why I was hopeful about the Community Initiative mentioned. But also it's why Alex requested refactoring things happen in its own follow up issue. Everyone wants it, but we also understands how much work that's going to be once you start lifting up various rocks and finding all the bugs and assumptions hidden underneath.
Catch's comment and re-opening of the original issue kinda made my heart skip a beat since that patch saved a site dying under the load of content moderation. I'll keep this short and just say glad y'all got to a solution so quick, thanks!
Added 2.x merge request so I can test it. Haven't tested it yet but the relevant code looks to be the same.
rebased as well to fix merge conflict.
last NW was from bot so moving back to NR for discussion.
re: #61, after this patch core doesn't provide that role on anything and your li class also doesn't match those provided in core so I don't think that's related to this issue. I would check your theme. This documentation will help you narrow down what template is causing that https://www.drupal.org/docs/develop/theming-drupal/twig-in-drupal/debugg... →
We had rough buy in for the previous approach including a CR documenting the change. Additionally I've been running the USWDS version of this for 2 years through several accessibility audits and its still the suggested solution provided by the USWDS.
So I think we should at least discuss before taking a completely different approach. Going to revert the current MR to the previous approach but feel free to open a different MR with the alternate approach to discuss.
That said, the exact suggestion you provided was (and it still) being discussed and so far has not been accepted.
Some users also suggested it made their SR experience worse https://github.com/uswds/uswds/pull/5197#issuecomment-2275809180
So unless we have a better reference for choosing the more verbose solution, I think it would be prudent to take their accepted solution and follow up if something changes.
you probably need assertions enabled to trigger it.
AssertionError: Cannot load the "editor" entity with NULL ID. in assert() (line 261 of /var/www/html/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php).
#0 /var/www/html/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php(261): assert(false, 'Cannot load the...')
#1 /var/www/html/web/modules/contrib/ckeditor5_premium_features/modules/ckeditor5_premium_features_productivity_pack/ckeditor5_premium_features_productivity_pack.module(89): Drupal\Core\Entity\EntityStorageBase->load(NULL)
#2 [internal function]: ckeditor5_premium_features_productivity_pack_preprocess_field(Array, 'field', Array)
....neclimdul → changed the visibility of the branch config_install_fatal to hidden.
Have a site with the config exported and with a clean database ran drush si --existing-config --account-name=admin --account-mail='info@example.com' --account-pass=admin
Merge fixed the problem for us by using is_syncing to skip all the config entity modifications. Since this is an install from config, the messages about composer didn't make sense to me so I put them inside the if statement as well.
Just tested on a site with ~250k user entities and before we where getting very slow responses and 500/OOM errors before and with the patch the responses are quick and we're no longer getting OOM errors.
larowlan → credited neclimdul → .
rebased and fixed a couple obvious things in the merge request but tests fail so there are additional changes needed. Can't get lando/poser working anymore and ddev-contrib can't run tests https://github.com/ddev/ddev-drupal-contrib/issues/140 so got stalled and ran out of time.
Will have to come back.
neclimdul → changed the visibility of the branch project-update-bot-only to hidden.
benjifisher → credited neclimdul → .
larowlan → credited neclimdul → .
Don't know what information is still needed. As noted by Tim, this is related to making the form builder behave consistently in 🐛 Do not allow form builder functions to return Response objects Needs work as well as I'll note #2367555: Deprecate EnforcedResponseException support → .
But both of those are moving slowly so there's not much that can happen here I don't think.
poker10 → credited neclimdul → .
"bugs affect non-cron queues as well"
Do you have an example? I've not run into this issue and there's conceptual explanation but not real world examples.
Looks like the subsystem tag was added for me and I added the review so removing the tag.
Its been a long time since I've looked at this but for us it wasn't critical but it was getting that way. From memory, expire wasn't working and we where getting a massive constantly growing store. Adding this patch and then wiping the store it seemed to resolve the issue.
Unfortunately, I don't have an environment to test this on currently. This script definitely would have been useful at the time to debug it though so hopefully someone else can dig in further.
I will say, even if expiry is working, the permTtl day typo probably is mildly critical since the value currently misses the day component of the calculation so its definitely not working for all cases.
Set this to 2.x because that's where I've tested this. Looking at the code this seems to behave identically in the 3.x branch but I don't a have site available to test it at the moment.
neclimdul → created an issue.
Ran into this as well. Reviewing the merge request, the two if statements are just approximating an empty check so simplified it.
Here are the steps I used to recreate this.
1. Create a media field on a node or paragraph if you don't have one to test. It doesn't seem to matter where it exists.
2. On the "Manage form display" tab select "autocomplete" for the widget and save.
3. Now set it back to entity browser and save without expanding the gear and setting up the field.
Without the patch, at this point two things are broken throwing both an undefined array key warning and a fatal type error.
1. From the manage fields tab editing the field is broken.
2. The node form or paragraph addition form seem to also be broken.
With the patch everything is fine
1. Manage fields is normal. I don't see any use widget language so this call might not be strictly necessary.
2. The form "works" but the user is presented with "Entity browser not found. You can select one media item." which would prompt a developer to fix the form display.
neclimdul → made their first commit to this issue’s fork.
benjifisher → credited neclimdul → .
I think I like this approach better but think this was fixed by ✨ Generalize MenuLinkAdd so all local actions can use it Needs work so we can close this.
neclimdul → created an issue.
neclimdul → created an issue.
Workaround is to save the organization which _does_ trigger this operation.
neclimdul → created an issue.
I thought the discussion on the mailing list was to not touch the functions and just drop the 32bit support
xjm → credited neclimdul → .
dang, needed contrib testing to work today and this is blocking it. That sucks.
Looks like an upstream fix is in the works to be released soon.
https://github.com/PHPCSStandards/composer-installer/pull/245
Filed against 6.2 as I've only had a chance to test it there so far but I don't see any obvious changes that would have fixed this in newer versions.
neclimdul → created an issue.
Talked to catch to clarify and we don't really need to push the method to the interface at all since this is dead code and it just adds complexity.
I shuffled Shalini's code around to accomplish this. Also bumped code and CR to 11.3 as well as point the unlikely user that finds this to the method on the storage class that still has this functionality.
Discussed this with catch he clarified to me that the proposed changes couldn't be backported without fatal errors in 10.x which meant that continued BC was not possible.
ugh, fighting with this again this week. Have to mock up a search indexing operation and xdebug to figure out why something wasn't going into the index correctly rather then just previewing and seeing what template was getting picked up. That's annoying for me, but we're going to have content editors working with the changes we're making and I have to explain it to people that "sorry, you can't preview how your content is going to be seen by search" which is going to be very unsatisfying for them.
Hope we can come up with a solution.
</rant>
borisson_ obviously is the expert on SearchAPI but I'm not sure if this _actually_ affects the module. Writing this I realized core just might be providing a trap for SearchAPI users and this is a bit of a problem of my own making. I used the out of the box `search_index` mode because it just made sense, but when choosing the rendered option in SearchAPI I can actually chose any display mode. So I could make `sapi_index` and label it "Search Index" and this whole problem probably goes away for me at least in the context of SearchAPI.
So I wonder what the point of the permission even is. Maybe there _should_ be some sort of generalized permission for display mode previews but are we concerned something might be leaked that's going into a search index specifically? Conceptually its information that's going in a publicly searchable location so you wouldn't put privileged information in it except for in very rare situation of internal or gated searches but even then the publisher's going to have access to the fields.
https://docs.phpunit.de/en/12.0/code-coverage.html#ignoring-code-blocks
Supported in future versions of phpunit so seems its still relevant.
benjifisher → credited neclimdul → .
I was really confused to see this being worked on but I see its replacing the multiple providers with dependencies which does seem like a good move. Not an actual review, just the approach makes a lot more sense.
Updating title to clarify the new approach and match updated IS.
Knew there was an issue about using buttons for more elements. Here's at least two
- 📌 Use form element type instead of Needs work
- 🐛 There is no way to make a button element that can use the AJAX functionality Needs review
Probably worth considering how those interact with this and can solve issues like what you're describing the right way rather than putting in another half measure in place.
I'm not sure announcing it in a changelog is enough.
First, its deprecated, not changed and 4.0 isn't out. So the expectation would be that deprecated code would continue to function issuing the warning to users.
`Not providing the "$view_mode" parameter is deprecated in flag:8.x-4.0-beta4 and will throw an error from flag:8.x-4.0. See
https://www.drupal.org/node/3458551 →
.`
Since it didn't, I just got a fatal error never having seen a deprecation. No chance to fix I i missed the changelog.
Second, the default code for generating links doesn't require a view mode and defaults to null.
/**
* Get the action link as a Link object.
*
* @param \Drupal\flag\FlagInterface $flag
* The flag entity.
* @param \Drupal\Core\Entity\EntityInterface $entity
* The flaggable entity.
* @param string|null $view_mode
* The flaggable entity view mode.
*
* @return \Drupal\Core\Link
* The action Link.
*/
public function getAsLink(FlagInterface $flag, EntityInterface $entity, ?string $view_mode = NULL);
I haven't tested it yet but this would seem to trigger a fatal error when using the function as documented.
oh... right... I'm certain there's a bug formapi kinda always using the input element for buttons/submit. I have always had a "is_real_button" element hacks to work around this on sites.
I don't see how the mouse up actually solves it since a right mouse button should also trigger a mouse up event.
rebased. Doubt back porting to d7 tag means much now so removing.
I suspect we need to fix media's buttons then. Without looking at the interface, I suspect they shouldn't be submit and just buttons since what your describing sounds like an interactive action, not a form submission.
What you're describing of triggering the first submit button is expected HTML behavior outlined in the HTML spec and I believe required by WCAG?
"The default assertion message provided by PHPUnit is sufficient for most situations."
Still strongly disagree with this.
Something like "Failed asserting that false is not equal to false." is entirely insufficient and such assertion failures are most situations.
poker10 → credited neclimdul → .
poker10 → credited neclimdul → .
Sorry for the late response. Not sure how related this is to the parent unless the scope of that issue is different then the title.
Technically we're already using the … entity so which is probably better then the UTF8 character. This is issue adds information to the pager to explain what an ellipsis means in the context of the pager list. Specifically explaining what the missing information is.
This came out of the original Drupal 8 initiatives. We where using sandboxes as a place to experiment and prep and collaborate around merge requests. There was no "merge" process at the time only patch acceptance so the idea here was to come up with a way someone making a complex change with multiple commits could get those merged instead of just committing a patch.
In a lot of ways this is resolved with issue forks, I wonder if there's a similar problem with squash commits vs preserving the history but I haven't run into the problem yet.
neclimdul → created an issue.
greggles → credited neclimdul → .
As I read your comment I think "Yes, the second example is much harder to read and gets worse as it gets longer because it becomes harder and harder to parse the groupings of the assignments at a glance." but clearly that's not what you means so I don't think we agree.
But lets take your points and address them.
- "The added burden of handling updates when something changes."
This has been pointed out repeatedly and refuted as not be a big burden, and these lists seldom change and the change isn't hard. As an example, the sqlite code actually needs to be updated, it took me about a minute to make the patch. Half of that was remembering if it was ctrl-shift for multi-line select in jetbrains because I switch editors a lot. Most of the time you're going to be adding the smaller value and its trivial. - "The longer the longest assignment is the further your eyes have to travel whitespace to find the assignment."
The longest gap in the updated sqlite it 13 characters. Previously it was 8. But that misses the point. The formatting means you don't have to scan back and forth, it lays it out in a big table so you can skim it and understand it at a glance. - "If the longest pushes it passed 80 characters, all would be beyond 80 characters."
The sqlite example maxes at 43 characters long.
The color example sited repeatedly is now quite a bit longer with language features, a long name, and a big arrayprotected const ROTATE_TRANSPARENT = [255, 255, 255, 127];. That reaches a max of 61. At least the examples in core this doesn't seem likely to be a problem unless its some _deeply_ nested code and we should probably refactor that anyways. - "Diffs for fixes space columns can be messy and hard to read, changing one requires all lines to update rather than just showing the addition or deletion."
This is a general problem for all white-space heavy changes. As the last couple comments pointed out these are not commonly changing so this is an edge case change but in the case you do find yourself having to make or review one of these changes, all git platforms have a wonderful solution that highlights exactly the changes you're looking for. Its going to save you a ton the next time you have to wrap some code in an if.- In Gitlab, on the top of changes tab in the preferences on the right you'll see hide whitespace changes. On commits its a "hide whitespaces changes" button.
- Github has is too its just in the gear drop down.
- Git locally you can provide -w to the diff and show commands.
Not really sure what the supporting user section is for but just also its been years just to be clear, still -1 for stricter rule. +1 for maintaining readability.
Morbus' recent example and the other examples in this thread are succinct for discussion but in reality most use is like the below code from our SQLite driver where the tabular format is useful for comprehension.
$magic_map_or_list = [
'varchar_ascii:normal' => 'VARCHAR',
'varchar:normal' => 'VARCHAR',
'char:normal' => 'CHAR',
'text:tiny' => 'TEXT',
'text:small' => 'TEXT',
'text:medium' => 'TEXT',
'text:big' => 'TEXT',
'text:normal' => 'TEXT',
// another like 20 lines mapping data types.
];
To joachim's point, these uses are often read and slowly changing.
For clarification of anyone catching up, Squiz.WhiteSpace.OperatorSpacing is actually in core today. We added it in #3099583: Update coder to 8.3.7 → replacing the Coder WhiteSpace checker, we just don't use the property related to this issue.
Fix is basically the same as the lazy builder. The check was already in place this just avoids the array key doesn't exist error by using ?? same as the lazy issue
neclimdul → created an issue.
neclimdul → created an issue.
Skimming the failures I see some interesting things including a lot of tests asserting relative url's in places I would have expected the API to want an absolute URL so my gut tells me this is maybe kinda needed by other places in core.
And while search_api could fix it, I feel like this is a common problem I run into so I'd like to push the fix to the source rather then having a ton of hacks.
But also that's just a ton of systems affected by failures and its going to take a long time to audit and review just the test changes that would be involved let alone documenting how this would help site developers and contrib.
So rather then just leaving this to clog up the queue I'm going to go ahead and close it as suggested and I'll revist it as I have time to review all that code and document why this is needed better.
Works for me. Back to RTBC?
Yeah I don't disagree with classifying search_index as not strictly a frontend view.
I think what I was trying to convey in the summary was that while Drupal we called the preview interface "Preview in live environment" in the original issue, but that is in no way conveyed to the site user. To a developer and content editor its just "Preview" so seeing how something will be pushed into a search index can provide a lot of valuable information.
So limiting the interface to strictly frontend displays doesn't always meet user expectations. At least it didn't meet mine.
Pushed a simple implementation. Testing still needed but its "reviewable"
neclimdul → created an issue.
Sorry I never followed up on those answers. I've been thinking about this and the general asset delivery problems datalayer, CSP, attach inline, as well as some personal issues I'm trying to solve. But, other than "core doesn't seem to be providing enough" I've not been able to come up with the best way to dig in to it yet.
also... been busy.
Should be a simple fix in the merge request
neclimdul → created an issue.
Didn't mean to change the title. I started to take a stab and didn't have enough caffeine in my system yet to have a good suggestion.
I think this is stalled pretty hard since the discussion is getting close to 5 years. However, I feel like there was pretty strong consensus around settings some guidance at least that messages should be positive/"confirmative"/affirmative additional messages, letting the assertion show the negative/failure information and the message provide the context for what the failure means.
In the interest of closing issues, could we more forward with just that and if there's apatite for removing or suggesting the removal that could be handled separately? I'm biased because I had strong feelings but I feel like that was always the part we where aggressively agreeing over.
benjifisher → credited neclimdul → .
This needs to be re-built now that symfony's listener is out of the mix and we're on phpunit 10. Its likely less complicated but I need to look at it.
Looks good for the straightforward fix.
Will leave questions of the not strictly camelcacse code and possible code removal to committer review and RTBC this.
poker10 → credited neclimdul → .
This actually shows up in the phpstan report as well.
I think this might actually be a dead method as well so removing it might also be an option. It just proxies through to the storage method of the similar name(different capitalization) and using that might be the correct way of accomplishing this.
neclimdul → created an issue.
longwave → credited neclimdul → .
Fix in the MR.
One bonus benefit of checking the display_id like this is we can completely delay loading the view executable until the lazy builder, further optimizing the lazy path and only loading it once.
neclimdul → created an issue.
Ran into one of the instances of this today and went "what is that word?!" and now I find myself here. So I guess I support converting to just "cast" I guess :-D
I was confused why we hadn't changed default.settings.php and the answer is that its not currently there. This is one of those power user settings we don't document and only set if you're running into an edge case.
I suspect that will continue and changing that would be its own issue but I understand how its tied up in this. I think we can mitigate this by linking to the documentation in the changelog and maybe release notes so in the unlikely case this does push someone into the edge case they know how to resolve it.
Also conflicted with 🐛 Performance Degraded after update to twig 3.14.2 Active
merge request made and passing so back to NR.
The performance regression changes where a bit disruptive. They make this change kinda BC breaking because now the settings are directly exposed in the less secure expression format. Which is really frustrating and I don't really know how to resolve it.
broken by 📌 Remove misc usage of whitelist in tests and comments Fixed . reroll soon.
Technically I think it was added in 8.3 or 8.4 so its pretty recent even in the SB8 releases. We're using the --no-open workaround.
Fixed