I believe the issue I experienced was something like this:
- A menu block is configured to display the menu for children of the current page (children which may or may not exist; or indeed the current page may simply not be in the menu at all).
- The block is assigned to a block region which has no other blocks -- it exists for the sole purpose of displaying that menu, and only if there is a menu to display.
- If the block's access check returns false, then the block is not included in the region, and the region is not displayed, as it has no contents.
- If the block's access check returns true, then the block is included in the region (with a minimum of some generic block boiler-plate markup), and the region is rendered.
- If the latter still happens when the menu was "empty", then we see an empty region/sidebar displayed on the page, effectively pushing the rest of the content out as if it were padding.
> I've added the following patch and it works to ignore /admin routes.
It seems to be ignoring authenticated users, rather than just /admin routes.
In any case, I would again recommend using ✨ Provide hook_seckit_options_alter() D8 Needs review to implement conditional behaviour. (That feature should be merged at some point.)
I note that the functional change in #74 has been lost (it wasn't included in #79).
https://www.drupal.org/project/redirect/issues/3057250#comment-15524338 🐛 Validation issue on adding url redirect Needs review
Perhaps that issue was dealt with in some other way in the interim. Either way, it probably needs to be reviewed.
/report-csp-violation is the relative URL to which web browsers submit violation reports.
To view the submitted reports, look at watchdog.
Very true, "other users" are only relevant to the persistent caches (I was considering both in the other issue).
I don't believe that renders the notion irrelevant, though. After all, no cache-expiry scheme has any advantage over any other if the cache doesn't fill up. We're only concerned about what happens when, for whatever reason, the cache fills up, and my suggestion is that giving the programmer some control over what should be flushed and what should be kept in those situations would be an improvement, and that a simple LRU approach isn't always going to be the best approach.
Just cross-referencing with another old issue: https://www.drupal.org/project/drupal/issues/1961934#comment-7274730 →
Back when I raised that I was seeing a pattern of entity cache problems, and observed that a LRU approach was similarly problematic in the circumstances in question ("With LRU and limited memory, you can easily throw out everything you want to keep"), so I proposed a way of providing hints to the cache to enable it to be smarter.
I haven't tried to catch up with what's currently happening in this issue, so apologies in advance if it's not relevant any more; but seeing as how there's current activity on this LRU caching issue, I thought I'd add a link in case it was still a going concern.
The git commit noise and conflicts are a result of multiple different users/IDEs applying different sorting rules, and re-arranging the order of lines which were previously sorted by a different program. That sort of noise can ping-pong back and forth if multiple developers are working on the same file (not just the same part of the file), because the list of Use statements at the start of the file is for the whole file. Enforcing a standard, well-defined, stable sort order ensures that this kind of noise won't happen (or at least won't ever be merged).
Just to be clear, it was also affecting people prior to the recent change. It's simply affecting different people since the change.
Reverting the change means that the original set of people get the problem again. Which is an option, for sure, but it's not making the problem go away.
You can use hook_seckit_options_alter() to modify the settings however you wish based on any arbitrary criteria, certainly including "this is an admin path". For the Drupal 8+ versions, you'll need
✨
Provide hook_seckit_options_alter() D8
Needs review
to provide the hook.
it's causing a major amount of work for us to update all of our projects to pass our linting/coder validation.
It sounds like a relatively easy Perl script to update them all in bulk, FWIW.
Excellent, thank you sokru.
There's really no cause for confusion. Backslash is ascii 92. Underscore is ascii 95. Hence backslash sorts before underscore.
I can't think of a locale we might be interested in where that order would flip (if any even exist). Certainly `C` and `UTF-8` locales sort these characters in ascii sequence, and I don't think there's any reason why we'd deviate.
printf '\\\n_\n' | env LC_COLLATE=C sort
printf '\\\n_\n' | env LC_COLLATE=UTF-8 sort
If VSCode is defaulting to the reverse when sorting lines of text, that just sounds like a VSCode bug/feature, either to be fixed upstream or configured to be more sensible (but I couldn't guess which is more likely).
FWIW, I would write the previous example as:
if ($long_condition_a
&& $long_condition_b
&& ($long_condition_c1
|| $long_condition_c2))
{
return 'yes';
}
I find the newlines-after-( approach painfully verbose (and ugly), whereas I find the above lays out the structure in an extremely readable fashion, without the waste of vertical space.
It's definitely important to have the { on a new line though (or the ) { if taking that approach), otherwise the body of the block isn't visibly indented from the conditions. So that's a must-have, to my mind.
Operators on the left is the one I'd fight for more than anything, though.
I am strongly in favour of the "keep left" approach to operators (i.e. operators at the start of lines).
I was trying to find a reference for this recently for a colleague, and the best I could find online is in the Python PEP8 coding standards:
https://peps.python.org/pep-0008/#should-a-line-break-before-or-after-a-...
I think they lay it out pretty clearly.
When you read left-to-right and all the operators at a given level are aligned on the left, then your brain has to do very little work to see the structure of that code. Conversely, if you have to scan to the end of each line to figure out how it fits into the structure, you're working comparative overtime.
A colleague recommended this to me a very long time ago, and I've never been so immediately convinced by a formatting change in my life. I've done it this way ever since, and would gladly see it be part of the coding standards here. It's just better.
Hi Kristen. Does that mean that my assessment in #17 was incorrect? The two issues didn't look to me like they were about the same thing.
Kristen Pol → credited jweowu → .
I think it's in scope insofar as it's a regression of sorts -- the unpatched module doesn't hinder the user in the situation I've raised because the unhelpful warning message was displayed automatically using AJAX, before the form was even submitted.
With the patch, the warning doesn't happen until the user submits the form, which then means they now need to submit the form a second time to make it stick, when previously once was enough.
I.e. Because we've removed the AJAX in this patch, I think we need to also remove any false-positive warnings.
I'm not sure what's going on in #72 as the issue fork commit is identical to #65.
This patch is the same as #65 plus these additional changes:
* Allow the user to add a redirect from an internal path to an external URL without an unhelpful validation warning to consider using a URL alias instead.
* Remove some use statements from RedirectSourceWidget.php (their usage having been eliminated in previous revisions of this patch).
I do appreciate the benefits of deferring to other tooling; I just think it was jumping the gun to remove the supported-module functionality before composer provided any equivalent. It's great that there's an upstream composer issue in progress which will ultimately fill the gap, but it's approaching 5 years since that issue was raised, and that is not an especially unusual lifespan for software issues generally (and nor is twice that duration for that matter), so IMHO a slightly more conservative approach to removing features would be justified.
I think efforts are better spent on getting Composer to work as you want. See #32. If you don't care to help issue that along, then use the composer extensions mentioned in #29, or make a contrib Drush command.
Noted, thank you.
Moshe: Regarding drush again (having now noted that you're a primary maintainer of that)...
I've tested reversing https://github.com/drush-ops/drush/pull/5775 in order to run drush sec against my codebase (after reinstalling the outdated version of the module via composer), and can see that I was incorrect in suggesting that drush sec had indicated outdated modules (and so I presume there's no benefit in restoring that code specifically).
My recollection of drush ups (pm-updatestatus) from older versions was partly accurate, though: Drush 8's pm.drush.inc and updatestatus.pm.inc show a broad range of status values it can associate with a project. However the --security-only option looks like it would have ignored unsupported modules.
I've been under the misapprehension that drush sec essentially did the same as drush ups --security-only, and that they'd both treated unsupported modules as at least potential security risks (which I think they should do -- a module which is unsupported cannot be expected to receive security patches, even if it contains the identical vulnerable code which was been disclosed and fixed in the supported versions of the module). It's also worth noting that admin/reports/updates treats unsupported modules as Errors in terms of severity, so there's really quite a disparity between the different information sources here.
Given that I cannot see any way in modern drush to list those details (pm:list looks like the only likely command in Drush 12, and it has no such options), it feels to me like a step backwards to ever have removed the pm:updatestatus functionality. Surely we want drush to be capable of providing the same information that the web UI shows at admin/reports/updates?
The deprecation message for pm-updatestatus says "Please see composer show and composer outdated" (in line with the earlier suggestion to use composer outdated "drupal/*"), but that command tells us nothing about support status, so it's not actually a replacement. For my example module it only tells me "patch or minor release available - update recommended"; and composer show drupal/simple_menu_permissions likewise has no understanding of the support status.
My vote would be to restore the pm:updatestatus command in some form, regardless of what may or may not be done with composer, and to provide options for listing known-insecure and unsupported modules. Drupal already knows how to generate that information, so to me it only makes sense to make use of that.
Can anyone verify that, please? If that's true (and going to remain this way) then we'll clearly want drush to restore the perfectly functional command it already had for this.
(Which will be pretty unfortunate, because developers everywhere will already have removed that drush command from their CI configs on account of CI breaking when the command was removed, and nothing is going to force them to notice when it is restored.)
Does this work as intended?
I have a Drupal 10.1 site currently warning me on its admin/reports/updates page that Simple Menu Permissions 2.0.0 is an unsupported module, and that I should upgrade it to version 2.1.0.
composer audit doesn't mention this at all. Seemingly it either knows nothing about unsupported modules, or else doesn't deem them worth mentioning.
I'm pretty sure the old drush sec and/or drush upc --security-only commands gave the same information that admin/reports/updates is telling me, but those drush commands have been removed in favour of the seemingly-oblivious composer audit command.
Hi there,
This was only ever a Documentation task for the module's maintainers to note *somewhere* that there are some alternatives to using this module, but I don't see any notes to that effect on the current project page or in its readme file, and a glance at the code of version 2.0 indicates that it's still using regular expressions to parse table markup; so I think nothing has changed.
Perhaps you could update the project page description to include the alternatives I described? If you did that, I'd consider this issue closed.
(And honestly, 12 years later I imagine there's no reason not to use the nth-child CSS selector.)
All contrib modules hosted on drupal.org are licensed under GPL 2+.
There is no need for individual modules to provide LICENSE files unless they wish to include other additional licenses.
Please refer to https://www.drupal.org/about/licensing →
Could this please be reviewed as a priority?
Per my comment in #35, it is really time-consuming to re-roll such a large patch. I spent the time on it in the hopes of seeing it merged at last, but I don't want to do it again. Please don't let it get stale again?
I don't understand the argument that making this an option is currently problematic.
Sure, in its current state such an option would mean "enabled for both ul and ol" or "disabled for both ul and ol", and some users might want only one or the other. But at the moment those users don't have any way to do that, so they'll be no worse off. Meanwhile the other users who want it enabled for both ul and ol also don't have any way to do that, and their requirement could be fulfilled. (And I'm struggling to imagine that the latter would not be the majority.)
This would in no way preclude a more granular config down the track, and in the interim it would resolve the issue for lots of people.
That was just a phrasing tweak for the code comments.
I don't know why it auto-marked that as a draft, but I'd hazard a guess that it was complaining about the fixup commit (which I think is dumb, because this then means I need to force-push changes, rather than trivially squashing the fixups once it's reviewed and without imposing force-pushes on anyone). Anyhow -- no actual changes there.
Great, thank you laryn.
We could also get rid of the is_string() check. Can the alias be anything else than a string?
Yes, at minimum the alias argument can be NULL. I can't recall offhand whether there are other possibilities, but there's at least that one, and the only thing the function can cope with is a string, so I think is_string() is the sensible guard.
That's 100% the intention of the patch, yes. It solves the problem raised in 🐛 Module causes double rendering of blocks just to check access Fixed without the side-effect of causing the present issue (and in turn causing people to use twig-based workarounds); so what you're seeing is exactly what's intended.
The pairing of a stable patch URL and a checksum of its expected contents is also safe, and significantly more convenient -- developers can safely share reviewed pairings without having to pass around the actual patch files.
For composer users, version 2 of cweagans/composer-patches supports this out of the box, aborting noisily if it fetches a patch which does not match the checksum specified in the patches.json file.
Well, there's a MR test failure for "PHPUnit Functional Javascript 1/2" which recurred on a retry, but there's no Javascript involvement here, so I'm setting this back to Needs Review.
Unless I'm missing something, the MR workflow more or less necessitates separate branches for testing against each version of Drupal as it's no longer a simple patch that's being applied, so I've created 10.2.x and 11.x branches for this.
Based on that issue, I'm not confident that the 10.2.x and 11.x core branches are going to pass their traditional testing anytime soon, so I've made a MR for testing #180 on those versions.
jweowu → changed the visibility of the branch 571548-identifiers-longer-than to hidden.
> ... delete unmerged branches once an issue is closed?
"Issue is closed" isn't a guarantee that users aren't using the branch.
That also relates to my question:
Can someone confirm that either (a) a patch file upload workflow will continue to be available under the new system; or (b) the new system provides equivalent functionality: persistent URLs for specific revisions of a patch (regardless of the current state of the MR/branch, and immune to git garbage collection).
It sounds like it's only the testing of patch files which will be disabled, so presumably we do still get option (a) sans testing; but that being the case, option (b) would be nicer -- otherwise users still have to deal with patch files, but now they have to deal with MRs as well, and there's no obvious connection between the patch URL and the testing that was performed for it.
Last I knew, the nearest merge request option was a patch URL for "whatever the MR head happens to be at the moment" and users were being told to download local patch files as a workaround (which I've been presuming is a temporary workaround because it's a bit rubbish by comparison). https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... → doesn't indicate any improvements in this area.
> Recommend turning to MR as patches are being phased out.
The MR workflow at Drupal continues to not provide immutable patch URLs which is hugely problematic, so I'm sure the old method isn't going anywhere while that flaw remains. So long as the traditional patch files are supported I'll prefer to keep using them.
> appears to have several test failures.
Of the tested Drupal versions (10.1, 10.2, 11.x) only 10.1 seems to be currently passing its own tests (sans patches), and against that version this patch passed testing too. AFAICT the vast number of test failures on 10.2 and 11 are entirely unrelated to this patch, and there doesn't seem any point in re-testing until those branches start passing tests on their own.
> Also issue summary should follow standard issue template.
Indeed, that would be good.
Re-roll of #179 for 10.2.x and 11.x.
Running PHPStan on *all* files. ------ --------------------------------------------------------------- Line core/modules/views/src/Plugin/views/query/Sql.php ------ --------------------------------------------------------------- 525 Variable $alias in isset() always exists and is not nullable.
Which is complaining about the pre-existing (not part of this patch) code in Drupal\views\Plugin\views\query\Sql::queueTable():
// If no alias is specified, give it the default.
if (!isset($alias)) {
$alias = $this->tables[$relationship][$table]['alias'] . $this->tables[$relationship][$table]['count'];
...
The "always exists" is certainly accurate, but on first look I'm not convinced by the "is not nullable" part. PHPStan must have a reason for claiming otherwise (and I'd tend to assume it's right and I'm wrong), but the reason is sufficiently non-obvious that I can only question the value of eliminating safeguard code on that basis.
By my reading, if we had two calls to queueTable($table, $relationship = NULL, JoinPluginBase $join = NULL, $alias = NULL) using the same table and relationship arguments, and if the second of those calls had a NULL alias argument, then that null could reach the isset() call. Joining to a database table twice with different aliases and join conditions is 100% a valid thing to do in a SQL query (I wouldn't even call it uncommon), so I see no reason why in principle this couldn't happen.
It does seem like an empty string alias shouldn't be considered valid, though (especially as the alias is used as an array key, and null array keys are coerced to empty strings), so I'm changing that !isset() to empty() so that we can catch that case and hopefully prevent PHP Stan from asking us to delete a pre-existing safeguard.
So let's give that a whirl (against 10.1.x initially).
Following up #166, I think my guess there was correct.
> It's not immediately obvious what field_test_data_i was originally ... Maybe it was "id"?
EntityReferenceRelationshipTest::setUp() does this:
// Create reference from entity_test to entity_test_mul.
$this->createEntityReferenceField('entity_test', 'entity_test', 'field_test_data', 'field_test_data', 'entity_test_mul');
// Create reference from entity_test_mul to entity_test.
$this->createEntityReferenceField('entity_test_mul', 'entity_test_mul', 'field_data_test', 'field_data_test', 'entity_test');
Which looks like it's making field_test_data a reference to a entity_test_mul entity, and field_data_test a reference to a entity_test entity.
Both of those entity classes declare:
* entity_keys = {
* "id" = "id",
* "uuid" = "uuid",
* "bundle" = "type",
* "label" = "name",
* "langcode" = "langcode",
* },
So id looks like the only property it might be if the name is just a concatenation of field name and property key.
And finally, my interpretation for the truncated name is consistent with other appearances of that comment for the field_data_test field:
3 matches for "Test the forward relationship." in buffer: EntityReferenceRelationshipTest.php
:
142: // Test the forward relationship.
: $this->assertEquals(1, $row->entity_test_mul_property_data_entity_test__field_test_data_i);
-------
:
222: // Test the forward relationship.
: $this->assertEquals(1, $row->entity_test_entity_test_mul__field_data_test_id);
-------
:
281: // Test the forward relationship.
: // $this->assertEquals(1, $row->entity_test_entity_test_mul__field_data_test_id);
So I think the way forward is:
use Drupal\views\Plugin\views\query\Sql;
...
// Test the forward relationship.
$alias = Sql::sanitizeAlias('entity_test_mul_property_data_entity_test__field_test_data_id')
$this->assertEquals(1, $row->{$alias});
The access check should be FALSE since the update is executed by the anonymous user...
AFAIK, any time that accessCheck(FALSE) is introduced to an entity query which did not previously have an accessCheck, you are changing the behaviour of that code. Are these behavioural changes on purpose? (Was the original code wrong?)
In the past, access was checked by default, equivalent to accessCheck(TRUE), so if the intention is to update the old code without changing what it does, accessCheck(TRUE) is appropriate.
The change record is: https://www.drupal.org/node/3201242 →
Thanks joseph. Your patch and 🐛 [ansi-sql compliant updates] - postgresql/mssql/mysql/sqlite password_policy_update_8304 Needs review appears to be about a problem with update 8304 introduced in version 3.2, whereas this issue is about a problem with update 8301 introduced in version 3.1. I'm glad to know of your issue/patch, but from looking at the comments there and the fact that you seemingly had no problems with 3.1, it's not obvious that the two issues are connected.
#165 applies just fine to 10.1. You can check 10.2 and let us know if it doesn't apply.
> the inconsistency makes this seem like a core bug at heart
On which note, can people please indicate the latest core version of Drupal in which they've experienced this?
I've seen it in Drupal 9.5, so I'm primarily wanting to know whether this is happening to anyone in any Drupal 10 versions?
It sounds like this is an ongoing problem.
There doesn't seem to be any movement in resolving this (unless #12 is the correct solution in all cases), and I never got any clarifications from my questions in #9, so my inclination was to stick with 3.0 until this is known to be resolved (especially when #11 suggests the problem may recur). As such, I'm attaching a patch for running version 3.0 on Drupal 10 (which may or may not be adequate -- I'm in the middle of an upgrade process and have not tested).
My understanding is that the issue was introduced in version 3.1 (possibly #2877040: Remove dependency on CTools → or some side-effect thereof), but the inconsistency makes this seem like a core bug at heart, and researching this error more generally has turned up (a) numerous similar cases, and (b) ✨ Provide an easier way for developers to identify entity/field definition mismatches Active and from there, https://www.drupal.org/project/schema_diff → which looks like it could be really invaluable for debugging this.
Could those experiencing this issue please utilise this module and report back?
The old default behaviour for entity queries was equivalent to accessCheck(TRUE), so this is a behavioural change (which should be explained in the issue if it's on purpose).
Ah, interestingly I seem to be able to build 10.1.x at the moment (including with patches), but my attempts to build 11.x (both with and without patches) have failed.
I saw the "can't cd to stm" error earlier, but my last attempt failed with:
This may be the error:
6569361f5d2bde9a88fdf200# /bin/sh -c cd "${DOCROOT}" && echo '$settings["file_private_path"] = "sites/default/files/private";' >> sites/default/settings.php
/bin/sh: 1: cannot create sites/default/settings.php: Directory nonexistent
FWIW I was getting this problem* consistently on November 10th (Friday), and still on the 13th (Monday) when I tried again. The following day I noted that it was fixed, and so I didn't bother to raise a bug report, but I see that it's happening again and so maybe it's not actually resolved? I mention the dates just in case the maintainers are unaware that this same issue was occurring earlier.
(*) And/or similar failures. I remember the "can't cd to stm" error quoted above, but IIRC also encountered a composer failure at least one of those runs. Most of my attempts were for a Drupal Core instance with no patches.
Needs Work on account of #42 as well -- it's not safe for the stylesheet to use :not([type]).
That's certainly the other option. I was hoping it wouldn't be needed, but there must be something else styling list-style-type for those list items in your case, meaning that you then need to override it. I guess it's going to be safest to assume that will be the case, even if it's not true for everyone.
There's metadata loss here, not "hard" data loss.
It's certainly data corruption and a hard loss of semantic meaning.
I'd agree that some HTML markup could be classed as metadata for most intents and purposes, but table markup is so integral to the semantic meaning of that content that I don't think you can consider it as anything other than "data".
This issue might be treated as less important on the basis of it being likely to affect a relatively small set of users, but for those affected users I think "data loss" is an appropriate description.
The patches in this comment are for Drupal 10.1.x, but I can only presume the issue I discuss below applies equally to 11.x.
* Patch 'a' is a re-roll of the (11.x) MR from commit ea2a4f1a1368ec4347419e87fd43ebeb3764a219 against 10.1.x.
* Patch 'b' is a modified version of the same (see below)
The interdiff is between the two. The modifications are removing all cases of:
ul:not([type]) {
list-style-type: disc;
}
ol:not([type]) {
list-style-type: decimal;
}
Those cause problems but I believe also shouldn't be necessary. User agents should be defaulting to those same styles when a list has no type attribute and no list-style-type style; so setting them explicitly should be a no-op at best.
At worst, the :not([type]) makes these selectors more specific than plain ul and ol and therefore pre-existing styles for the less-specific selector get overridden. This is bad when, say, a list was previously being styled to have a list style of none and winds up with disc instead because it didn't have a type attribute in the markup. Using Claro as my admin theme, I was observing that issue in practice in, for instance, the Operations drop-down menus at admin/content.
> When I sort lines in PHPStorm they are sorted case sensitive. If this is a setting somewhere I do not recall setting it nor have I found it. This sniff is enabled for commerce_migrate and I could not use PHPStorm to sort the lines to pass phpcs. Am I the only one using PHPStorm that has this behavior?
Looking back at ✨ Add sniff to check and fix the order of Use statements Fixed I think it was https://www.drupal.org/project/coder/issues/3310013#comment-15101090 ✨ Add sniff to check and fix the order of Use statements Fixed which had informed my comments here. I presume that will explain what you're seeing.
Agreed. My recent patch in
🐛
Menu block renders when tree is empty as of 8.x-1.8 [regression]
RTBC
restores the blockAccess() method like the patch in this issue, but additionally adds caching to retain the performance improvement which had motivated the removal of that method, so it would be great if people using the patch from here could test my patch in the other issue.
My thought in #38 was that as the "case insensitive" decision appears to have been based on PHPStorm's default (but fairly bizarre) behaviour, we should change it if there's a way to tell PHPStorm via some config file in the Drupal repository to use case-sensitive sorting.
Without such a config file to automate the change for PHPStorm users they would presumably need to make a config change manually, in which case users will still encounter inconsistencies in this area -- just not the same users as before (I imagine it would be an improvement for pretty much everything other than PHPStorm).
Overall, case-sensitive is clearly correct, so in principle we should be using that, but I agree that the case-insensitive sorting of 'use' statements is highly unlikely to cause instability in practice, so it might just come down to "which alternative annoys the fewest users?"
(And on that note, I'm only guessing that PHPStorm is pretty much on its own in defaulting to case-insensitive sorting behaviour. I'm honestly just surprised that even one editor is doing that, and so I'm assuming it's probably only one, but I don't know that for sure.)
Is there an upgrade path between 2.x and 3.x, or should we expect that to be a manual process?
The template committed here seems bad to me.
Bootstrap contains the following:
.media {
display: flex;
align-items: flex-start;
}
And the new template sets the class "media".
As this is a Bootstrap-based theme, that's quite a change to suddenly impose.
Was this specific interaction intentional?
If not, please change the class in the template to something different...
jweowu → created an issue.
Or revert and add static caching for the build() output, so access builds the block, and calling build() reuses the cached output.
I believe this is the correct solution. I've implemented it in https://www.drupal.org/project/menu_block/issues/3271218#comment-15268732 🐛 Menu block renders when tree is empty as of 8.x-1.8 [regression] RTBC
#17 did not resolve the issue for me nor some other users.
This new patch implements the caching approach described in comment #26 above (and in #15) to deal with the performance concern raised in
🐛
Module causes double rendering of blocks just to check access
Fixed
, and restoring the blockAccess() method to fix the bugs described in this issue.
In principle that could all be refactored so that the expensive thing only happens if necessary, after establishing all of the cheap tests. This should be a worthwhile improvement (at a slight cost of readability) -- it would mean some users would simply never encounter this issue at all (when they will do with the current code), which represents a performance improvement even in cases where that count query wasn't taking down the server.
And here's a patch (untested) which does that.
The idea of this one is not to change the intended behaviour of the module, but defer that count query until absolutely necessary, meaning that it doesn't happen if it doesn't need to happen.
If I haven't messed up, I think this could be committed to the current code base as a general performance improvement which would also bypass the major problem in some cases.
This patch implements the workaround suggested in #4. It is not a general fix, and may not be appropriate for all users; but if the current code is killing your server, it's going to be an improvement.
In my case I have no entity reference fields which require a value, and so the entire purpose of the code which triggers the server errors is moot.
For reference, the relevant logic is as follows, and it's all in service of that final $may_remove value.
/** @var \Drupal\Core\Entity\EntityReferenceSelection\SelectionInterface $handler */
$handler = $this->selectionManager->getInstance($options);
$have_multiple_existing_entities = $handler->countReferenceableEntities() > 1;
[...]
// Determine if a reference may be removed.
// Unless the user has permission to delete the entity, then they should
// not be able to remove it if that will lead to its deletion.
$may_remove_existing = $settings['removed_reference'] !== self::REMOVED_DELETE || $entity->access('delete');
// Don't allow a user to remove the only entity if an entity is required
// and the user cannot replace the entity if they remove it, because
// this would put the form in an unrecoverable state.
$can_replace_last_reference = $allow_new || ($allow_existing && $have_multiple_existing_entities);
$reference_is_not_required = !$element['#required'] || $entities_count > 1 || $can_replace_last_reference;
// Unsaved entities may always be removed.
$may_remove = empty($entity_id) || ($may_remove_existing && $reference_is_not_required);
In principle that could all be refactored so that the expensive thing only happens if necessary, after establishing all of the cheap tests. This should be a worthwhile improvement (at a slight cost of readability) -- it would mean some users would simply never encounter this issue at all (when they will do with the current code), which represents a performance improvement even in cases where that count query wasn't taking down the server.
The checkboxes become unresponsive after the first time selection.
Someone please correct me if I'm wrong, but AFAIR that was one of the pre-existing problems with the original code. Maybe triple-check that you actually have the patch applied in the code you're running / caches rebuilt / etc...? I would presume that the work here to date does address that issue.
No, I think everything else that came up already has its own issue at this point.
Merge requests are merged through the drupal.org UI and they are always squashed and the commit message defined in the issue is used.
Oh, wow... that's awful. I know that's how things have always worked for patch files as almost invariably there is just one patch file, but I just assumed that with the advent of gitlab and merge requests there would also have had been option of merge commits!
With the testbot regularly rejecting contributions over unrelated changes, such unrelated changes are inevitable in the process of getting patches a green light; and even without unrelated test failures, large change sets are typically better as a series of atomic commits.
Thank you for clarifying this. I can't fathom why anyone made that decision, but it's good to know.
Also: Thank you for getting a new release out so promptly.
In general I wouldn't ever try to review a MR as a single diff unless there was only one commit, so I suggest always checking the commits list of any MR.
I should add that the same applies to squashing commits at merge time. I'm mostly sure I configured that MR to not have its commits squashed, but I see that squashing has occurred, which means that anyone subsequently reviewing the code history doesn't get the benefit of the individual commits. Whether or not to squash commits for a MR ought to be considered on a case-by-case basis -- sometimes it makes total sense, but in other cases squashing only destroys useful information for no benefit.
Thank you Berdir!
And last, all the coding standard changes make this a *lot* more complicated to review, as the patch/MR is 3-4x as large as it needs to be, so a reviewer needs to go through and find the actually relevant changes. The general rule is to only fix coding standard on lines you need to change.
Ah, sadly you didn't notice that there were three separate commits, specifically for that reason :/
https://git.drupalcode.org/project/pathauto/-/merge_requests/55/commits was intended to be extremely easy to review.
I'd noted in #58 that "I've split it into three commits (and hence I've used a fork/MR): one for the testbot complaints, one for the actual issue patch, and one for the additional phpcs fixes."
In general I wouldn't ever try to review a MR as a single diff unless there was only one commit, so I suggest always checking the commits list of any MR.
Remove a troubleshooting note which was made redundant by a recent edit → (and in future by 📌 Make commit-code-check.sh POSIX compatible Active ).
A more-palatable variant of the temporary table suggestion has occurred to me. I don't know how practical it is, but in principle I think this avoids the down sides of the other approaches mentioned.
In essence, while the cache rebuild is taking place, new cache invalidations get written to a temporary table. Then, after the cache rebuild, the cachetags table is truncated, and the rows of the temporary table are inserted.
In order for that to work, cache lookups need to know about the temporary table, something like:
if (a cache rebuild is in progress) {
check the temporary table for cache validity
if (the temporary table contained a row for that cache id) {
return result;
}
else {
// nothing about this ID in the temporary table
check the regular cachetags table
return result
}
}
else {
// not currently rebuilding the cache
check the regular cachetags table
return result
}
And similarly, when invalidating a cache entry the new invalidations value written to the temporary table would be an increment of the value in the temporary table if a row existed there already, and otherwise an increment of the row from the original cachetags table.
The lookups during cache rebuilds could be on a join of the two tables, rather than two separate look-ups.
No timestamp column needed, and no wholesale copying of cachetags; and outside of cache rebuilds the behaviour can be much the way it is at present.
Is that practical? I won't be surprised if I'm missing something, and I haven't thought through the ramifications of multiple simultaneous cache rebuilds (if that's currently permitted to happen), but it seemed worth suggesting.
Amavi: Is that on account of the cachetags table specifically? If a normal cache rebuild fixes things for you -- if temporarily -- then it's definitely not about cachetags (as the entire reason for the present issue is that cache rebuilds do not purge the cachetags table).
Your database will have many different cache tables, and I suspect your problem is something different to this issue. You should start by confirming which specific cache table(s) are getting so large, and then you can look for or post an issue related to that.
> This is such an extreme edge case/race condition that it can probably be ignored.
I can only assume that wasn't the conclusion when this was implemented, though -- it doesn't seem plausible that this issue never came up in discussion at the time.
> A workaround would be a cache tag last garbage collection timestamp to compare against
Yeah, I was also pondering that approach in #12 and #13. I think it's a pretty reasonable idea.
An alternative would be to have a pre-purge process which copied the cachetags table to a temporary table, and then post-purge deleted all cachetags matching an entry in the temp table. That has its own noteworthy costs, but they're isolated to the time of the purge. If the table was really gargantuan, though, it might not be great (and at the point in time when a fix is deployed, some sites are inevitably going to have tables fitting that description). Offhand I think I'd lean towards the timestamp column.
Per #86, setting this to Postponed pending 🌱 [policy] Standardize how we implement in-memory caches Needs work .
(And once that is resolved, the previous changes made for this issue ought to be reviewed.)
commit-code-check.sh is a bash script using bash-specific features.
Indicate what "yarn" is and where one might obtain it.
Just to summarise the last set of tests wrt recent discussion:
Passed for Drupal 10.1 (the only currently-testable version for this project):
PHP 8.1 & pgsql-14.1, D10.1 48 pass
PHP 8.1 & pgsql-13.5, D10.1 48 pass
PHP 8.1 & MySQL 5.7, D10.1 48 pass
PHP 8.2 & pgsql-14.1, D10.1 48 pass
PHP 8.2 & pgsql-13.5, D10.1 48 pass
Failing on account of unrelated issues testing Drupal versions < 10.1:
PHP 7.3 & MySQL 5.7, D9.5 46 pass, 2 fail
PHP 8.1 & pgsql-13.5, D9.5 46 pass, 2 fail
PHP 8.1 & pgsql-13.5, D10.0.7 46 pass, 2 fail
Failing on account of unrelated issues testing with MySQL 8:
PHP 8.2 & MySQL 8, D10.1 43 pass, 1 fail
PHP 8.1 & MySQL 8, D10.1 43 pass, 1 fail
PHP 8.2 & MySQL 8, D10.1 43 pass, 1 fail
Tests have now passed.
I'm not used to the MR workflow on d.o., but it seems as if "draft" MRs aren't (and can't be) tested, and fixup commits auto-trigger the change to "draft" (despite this MR being marked for eventual commit squashing); so I've squashed and rebased and force-pushed that, and set the MR back to "ready" in order to be able to re-test.
#120 is maybe a going concern?
Re-roll of #129 against Drupal 10.1.x.
(#134 accidentally included an empty file at a deleted file path.)
Re-roll of #49 against 10.1.x.
Re-roll of #129 against Drupal 10.1.x.
I've hijacked the no-op patch in this issue for the purposes of running tests to confirm that a failure I was seeing in 🐛 Duplicate alias entities created with 'Create a new alias. Leave the existing alias functioning' setting RTBC when testing with MySQL 8 was not on account of the patch in that issue.
I've added these tests to the patch in #2 here, which I expected to all fail:
* PHP 8.2 & MySQL 8, D10.1
* PHP 8.1 & MySQL 8, D10.1
* PHP 7.4 & MySQL 8, D10.1
(I'm surprised I was permitted to use that last combo, so that will probably fail more comprehensively than the other two.)
My extra tests here failed as well. The common factor seems to be MySQL 8, so it looks like there's an issue with pathauto and MySQL 8? (There's no test option for PHP 8.2 and MySQL 5.7 though, so I can't completely isolate it to that.)
Excellent, thank you xurizaemon; duly ignoring results for versions < 10.1 and I've queued up another couple of 10.1 test runs.
I see there was a failure for "PHP 8.2 & MySQL 8, D10.1" previously, so I'm also re-testing that in case that was temporary. It's probably not temporary, but I think not connected to this patch either, so I've queued up a test with the same parameters → on your no-op patch in 🐛 Failing test in module's existing coverage prevents unrelated patch submissions passing tests Fixed to verify whether that's the case.
...and in fact that's finished already with the same failure; so I believe we can also ignore that one for the purposes of this issue.