Thanks for the patch. The message says "missing or invalid", but it is only actually checking for values that are NULL or not set at all. It does not check if a value is an integer, which would be the only valid value.
It's a very simple fix. As it is, when Drupal 12 comes out, the user could get version 1.0.x.
This merge request is a temporary fix. It uses NULL instead of zero for the count. Zero is usually not the correct count while NULL represents an unknown value. It currently makes no difference, but combined with 🐛 Allow Result object to be created with NULL count Needs review it has the effect of hiding the count. Since the count is not available anyway, it is better to hide it instead of showing a count that is wrong.
liam morland → made their first commit to this issue’s fork.
It would be a good idea to drop support for unsupported versions as part of this.
liam morland → made their first commit to this issue’s fork.
Here is a start:
Using a sub-query ensures all query parts hit the index, greatly improving speed. Do not change this without testing with thousands of revisions and millions of entities.
The theme Government of Canada Design System (GCDS) → Intends to be a full implementation.
(I updated the code sample so that it is how it appears before the JS runs. The hydrated class gets added by JS.)
The latest revision ID does sound like a reasonable thing to cache.
So it sounds like one way of doing the query is slow with large numbers of revisions and the other is slow with large numbers of nodes. Could this be sped-up by adding indexes? Do we need to have a way of detecting or configuring the choice between the two query styles based on which would be faster?
Please put the patch into an issue fork and merge request.
This can likely be included with the other image issue.
If this is still a problem on a supported version, please re-open and provide details.
I have rebased the merge request and adjusted the tests so that they now pass. I am not sure that the test changes are correct; it may be that the test was right and the code is wrong.
liam morland → made their first commit to this issue’s fork.
Yes, that is the current state of things. Tests pass on 6.2.x but only because one test assertion is commented-out. The merge request in this issue re-enables it.
Is this still a problem? Are you able to write a test that surfaces the problem?
When you added that, there was a whole test file that had been deleted because it was failing. That is now back in.
They are running concurrently, but take almost the full half-hour so they could sometimes run over. It's also failing on the test assertion that is re-enabled by this merge request. See the previous test run for that failure.
After committing some small changes, test are currently passing on 6.2.x, however one test assertion is still disabled. The merge request now re-enables that assertion, so the merge request fails. Adding the dependency on jquery_ui_controlgroup did not fix it. I think this should be fixed first then the version of Drupal that tests are run in should be advanced.
I suggest that the next release be 6.2.0 and that it drop support for Drupal core versions that are end-of-life.
The development branch is 6.0.x; there is no 6.1.x. What is needed is a new tagged release.
jquery_ui has minimum of Drupal 9.2 so that could be used as the minimum for tocify too.
Link to module page and adjust spelling to match
I don't see how adding the return type declaration would cause a problem. Having not return type declaration means it could return anything. The merge request just narrows the possibilities. Adding the return type declaration to the interface could cause a backwards-compatibility issue. Better to add it to the implementations first.
The merge request makes static calls to instance methods. Here is an example of how to fix this.
@dmitry.korhov The test should be something that surfaces the problem. It would fail without your change and pass with it.
OK, that sounds like a reasonable thing to do.
Are you suggesting dropping the dependency on cl_editorial and having it detect and behave differently based on which modules are installed, cl_editorial and/or ui_patterns?
@dmitry.korhov Are you able to add tests to merge request 13526?
Is this the same issue as 🐛 Get SDC components inserting Active ?
We have this installed on a couple of projects.
I opened a merge request with the patch in #2.
liam morland → made their first commit to this issue’s fork.
I opened a merge request with the patch in #2.
liam morland → made their first commit to this issue’s fork.
I opened a merge request with the patch in #2.
liam morland → made their first commit to this issue’s fork.
You don't need a new MR. You can rebase the branch that is used by the MR and force-push it. GitLab will generate a tag pointing at the old branch location so the commits remain available for reference.
I note that LICENSE.txt was added. This gets added by the packaging script so it shouldn't be added in the repo.
A 5.0.x development release node should be created.
# version: VERSION can be removed from the info file.
Is this module confirmed to work on Drupal 11 with the change in this issue?
Tests are currently not passing on the development branch, so they won't pass on merge requests either (unless the merge request contains a fix). For now, try to ensure that the merge request doesn't create any more failures.
The merge request should have no merge commits, just commits on top of 6.3.x.
The merge request needs to be cleaned-up. It needs to contain just the few commits that are specific to this issue.
If the fillpdfservice issue is fixed, this may be re-opened.
If the fillpdfservice issue is fixed, this may be re-opened.
I don't understand what this is about. Is this still relevant in a supported version?
Sounds like this may have only been a problem on Drupal 7. If not, please re-open and provide details.
It sounds like a solution has been found. If there is a bug here, please re-open and provide details.
Presuming fixed. If not please re-open and provide details.
You may re-open this and provide steps to reproduce the problem.
You may re-open this and provide steps to reproduce the problem.
You may re-open this and provide steps to reproduce the problem.
The patch does apply to 10.5.x as intended.
I have opened a merge request with the fix previously provided in 🐛 Entity queries querying the latest revision very slow with lots of revisions Needs work .
I have put the same code into merge request 13453 on 🐛 Updating to 10.5.3 causes gateway timeouts on revisioned content Active .
liam morland → made their first commit to this issue’s fork.
I have adjusted the test in merge request 13268. I think the tests will now pass. I'm not certain that the test was wrong. It may be that the test was correct as it was and the code is not doing what it should.
liam morland → made their first commit to this issue’s fork.
Please open a separate issue for the issue with the spaceless filter.
liam morland → made their first commit to this issue’s fork.
It's documented in Let Drupal know about your module with an .info.yml file → .
There is no need to have php: 8.1 in the info file. PHP 8.1 is already the minimum for Drupal core.
core_version_requirement needs to be updated to match composer.json.
You could either cherry-pick all the needed commits or use git rebase --onto. Then force-push the branch and update the merge request to target 6.3.x.
liam morland → made their first commit to this issue’s fork.
@paulguy it would help a lot of you can use git bisect to determine which commit between 10.5.2 and 10.5.3 caused the problem.