I will update the title and confirm the steps in the updated summary by EOD Monday 2024-07-29.
- 🇬🇧United Kingdom 2dareis2do
I guess the issue will never be fixed in core if the issue queue is now in contrib.
- 🇬🇧United Kingdom 2dareis2do
Right sorry. my bad.
Am i right in thinking this started as a core issue?
- 🇫🇷France fgm Paris, France
Note that this issue is about the contrib statistics project, not statistics in core, but the link you mention https://github.com/drupal/drupal/tree/10.3.1/core/modules/statistics is about core.
- 🇺🇸United States mherchel Gainesville, FL, US
The
3250234-back-to-top-mikes-sdc-branch
above is ready for review. I added a JS fallback for older browsers and fixed some linting.Note the scroll progress only works in Chromium as progressive enhancement. I don't want to add a scroll event, which has the potential to slow down the browser.
@ehsann_95 I see you're still putting in a bit of effort on the other branch. You're welcome to continue to do so, but there's still quite of work to be done. You're welcome to review the new MR, which should be a bit closer to complete.
- 🇬🇧United Kingdom 2dareis2do
Patch for 10.2 does not apply to 10.3 afaict.
After updating to 10.3.1, I am no longer sure if this patch is required?
Certainly the other dependent issue appears to be marked as closed.
Disabling for now.
- 🇬🇧United Kingdom 2dareis2do
Looks like there are quite a few change in statistics module. with 10.3 release.
https://github.com/drupal/drupal/tree/10.3.1/core/modules/statistics
Patch does not apply and statistics are not updating.
Patch probably needs to be re rolled.
- @prem-suthar opened merge request.
- 🇮🇳India Prem Suthar gujrat
Prem Suthar → made their first commit to this issue’s fork.
- 🇧🇪Belgium herved
This seems to relate/overlap with 🐛 Error: Call to a member function uuid() on null in Drupal\media_library\MediaLibraryEditorOpener->getSelectionResponse() (line 72 of /app/docroot/core/modules/media_library/src/MediaLibraryEditorOpener.php) Needs work .
The MR there seems to fix it but I haven't compared the code of both solutions yet. - 🇳🇿New Zealand quietone New Zealand
Adding what this is postponed on to the remaining tasks per Remining tasks → .
- 🇮🇳India sukr_s
I've updated the comment. Splitting it into two methods will not help since the $args should not have expression placeholders. so the first loop will have to be repeated again or additional checks done to exclude the expression placeholders.
- 🇮🇳India sukr_s
- Changed the title.
- Couldn't locate the MR comment. Nevertheless added a comment at the place of change. Automatically closed - issue fixed for 2 weeks with no activity.
- 🇺🇸United States douggreen Winchester, VA
I still get a PHP warning in AttributeArray (line 79) because the original input array is statically cached in ViewExecutable::getExposedInput() on line 726, which happens before the data is validated. We need to validate the data before it is saved.
- 🇳🇿New Zealand quietone New Zealand
Closed some issues that I think are duplicates, no credit to transfer. Also hiding files.
- 🇳🇿New Zealand quietone New Zealand
@sukr_s, thanks for working on an older issue!
I read the issue summary, comments and the MR. There are no unanswered questions.
I left a comment in the MR asking for a comment to be changed. The title here needs to be updated because it refers to 'password' but the MR makes no changes to password, only to password_confirm. Both of those should be simple to complete.
- 🇳🇿New Zealand quietone New Zealand
Trying for a better title. On scanning I kept thinking this was a regression.
- 🇺🇸United States mherchel Gainesville, FL, US
I created a new branch using SDC and CSS only (using scroll animations).
Right now the scroll stuff only works on Chromium. The main functionality works on all browsers.
That being said, I think we might still want to use Intersection Observer so that the show/hide works also works on all browsers.
- @mherchel opened merge request.
- 🇳🇿New Zealand quietone New Zealand
@Berdir, thanks for the review. I responded to all the comments.
Good point about the novice tag, so I will remove it.
- 🇳🇿New Zealand quietone New Zealand
quietone → changed the visibility of the branch 3094865-fix-references-to to active.
- 🇳🇿New Zealand quietone New Zealand
quietone → changed the visibility of the branch 3094865-fix-references-to to hidden.
- 🇨🇭Switzerland Berdir Switzerland
Reviewed. Tagging this with "Novice" seems.. brave, lots of special cases here to think about.
- 🇺🇸United States kentr
container-inline.module.css
could also be moved, and then attached somewhere in thecontainer
chain. - 🇫🇷France zenimagine
Nice work. For the animation, how will you handle the infinite scrolling pages? Maybe by disabling the feature?
- 🇫🇷France nod_ Lille
Committed and pushed bd6abf7c4d to 11.x and d890962445 to 10.4.x. Thanks!
- 🇺🇸United States dblanken
dblanken → changed the visibility of the branch 10.1.x to active.
- 🇺🇸United States mherchel Gainesville, FL, US
Left some general comments. We should also move this to a single directory component. It's a perfect use case.
- 🇧🇬Bulgaria pfrenssen Sofia
Yeah this is not a bug, but a configuration error on the user's behalf. I hit this on a CI environment because the translations folder was missing.
I guess we can improve the error message.
The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
Automatically closed - issue fixed for 2 weeks with no activity.
- 🇭🇺Hungary mxr576 Hungary
Thanks @quietone for your high quality review again, I have fixed your findings, even more https://git.drupalcode.org/project/drupal/-/merge_requests/8317/diffs?co....
- ivnish Poland
Drupal 10.3: if your filters don't save, needs to disable cache for this view
- First commit to issue fork.
- 🇮🇳India akashdab
I think the before/after images attached previously did not highlight the issue and the solution, uploading a new set of images for better consideration.
- 🇮🇳India Chandansha
I've tested MR 8872 with the help of View live preview link.
i attached the video for refference.
I move it to RTBC.
THANKS!! - 🇳🇿New Zealand quietone New Zealand
I read the IS summary, the comments and the MR. The proposed resolution is out of dateAll questions are answered and the threads in the MR are correctly resolved.
I then applied the diff and tested. This works as expected and is an improvement!
I do think there follow up work.
- Add this feature to media files as well.
- The warning message uses the string 'It is advised' which is the first occurrence in core for a warning message. Because of that I think that the usability folks should review the string.
- The first time I read "It is advised to store file uploads for contact forms as private files. You can configure this in settings.php" I read the first as something to action so I tried to. Of course, the option is not available and I knew that but I still tried! So, again, I think the a usability review would help.
I think the above items can be done in followups. I am adding the tag for that.
I finally read the test. Is there a reason it is only testing 1 code path?
- 🇳🇿New Zealand quietone New Zealand
I read the IS and the comments. Thanks for the up to date issue summary. There are no unanswered questions but there are changes to core/modules/views/tests/src/Kernel/Entity/RowEntityRenderersTest.php that alexpott said are not necessary. They are still in the MR. And then, looking at the MR I left a question about changes to another file.
I read the change record and it is easy to follow. It is good that includes what sites should do. Nice work. At first I thought the title could just say that the link is not displayed instead of 'restricts'. But maybe that is a good thing so people are curious and read the notice.
Setting to NW for the 2 comments in the MR.
- 🇳🇿New Zealand quietone New Zealand
I read the IS, comments and MR. There are no unanswered questions. My question about the tests in #22 was answered but then later the tests were combined. That seems OK in this case.
Leaving at RTBC. Added follow up 🐛 Make label and category properties for layout plugins Active per #78 🐛 Add missing category to Drupal\layout_builder\Plugin\Layout\BlankLayout and let modules and themes alter the list of layouts RTBC to make sure label and category keys are set.
The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- 🇺🇸United States agentrickard Georgia (US)
The MigrationDeriverTrait only has one method.
Removing the Trait call and moving that method into FileMigrationDependencyManager.php fixes the issue, but I suspect this may affect all uses of the Trait.
It seems to be a race condition regarding how the plugin alter hook gets called.
- 🇺🇸United States agentrickard Georgia (US)
The latest patch introduces a hard dependency on migrate module inside file.module. That is not really acceptable, as it can cause errors in install from config.
Fatal error: During class fetch: Uncaught ReflectionException: Class "Drupal\migrate\Plugin\MigrationDeriverTrait" not found while loading "Drupal\file\FileMigrationDependencyManager". in /var/www/html/vendor/composer/ClassLoader.php:576 Stack trace: #0 [internal function]: Composer\Autoload\ClassLoader->loadClass('Drupal\\file\\Fil...') #1 /var/www/html/vendor/symfony/config/Resource/ClassExistenceResource.php(76): class_exists('Drupal\\file\\Fil...') #2 /var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php(361): Symfony\Component\Config\Resource\ClassExistenceResource->isFresh(0) #3 /var/www/html/vendor/symfony/dependency-injection/Compiler/RegisterAutoconfigureAttributesPass.php(32): Symfony\Component\DependencyInjection\ContainerBuilder->getReflectionClass('Drupal\\file\\Fil...', false) #4 /var/www/html/vendor/symfony/dependency-injection/Compiler/Compiler.php(80): Symfony\Component\DependencyInjection\Compiler\RegisterAutoconfigureAttributesPass->process(Object(Drupal\Core\DependencyInjection\ContainerBuilder)) #5 /var/www/html/vendor/symfony/dependency-injection/ContainerBuilder.php(767): Symfony\Component\DependencyInjection\Compiler\Compiler->compile(Object(Drupal\Core\DependencyInjection\ContainerBuilder)) #6 /var/www/html/web/core/lib/Drupal/Core/DrupalKernel.php(1447): Symfony\Component\DependencyInjection\ContainerBuilder->compile() #7 /var/www/html/web/core/lib/Drupal/Core/DrupalKernel.php(971): Drupal\Core\DrupalKernel->compileContainer() #8 /var/www/html/web/core/lib/Drupal/Core/DrupalKernel.php(515): Drupal\Core\DrupalKernel->initializeContainer() #9 /var/www/html/web/core/lib/Drupal/Core/DrupalKernel.php(739): Drupal\Core\DrupalKernel->boot() #10 /var/www/html/web/index.php(19): Drupal\Core\DrupalKernel->handle(Object(Symfony\Component\HttpFoundation\Request)) #11 {main} in /var/www/html/web/core/modules/file/src/FileMigrationDependencyManager.php on line 18
- 🇺🇸United States smustgrave
Checked the new modules being installed and seems they are indeed needed.
Looks like a good conversion.
The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- 🇺🇸United States DamienMcKenna NH, USA
The merge requests need to be updated to the latest changes on their respective branches.
Removed Functional test case FieldEntityTest as kernel test added & made respective amendments to kernel test.
This issue tagged novoice, however this issues takes time to convert it to kernel test, as there are many errors & to address those added respective API function calls & also removed the novoice tag as it is bit challenging to fix it.
Please review MR 8901, moving to NR.
- 🇮🇳India nishtha.pradhan
Hi @Sourav_Paul, my apologies, I am new to contributing, so was not aware of the standards.
Will keep this in mind, thanks. - @pooja_sharma opened merge request.
- First commit to issue fork.
- 🇦🇲Armenia Murz Yerevan, Armenia
> Appears to have phpstan issues
Yes, but they are not related to the changes at all. And I have no idea about how to fix this error, only remove the test. Here is an issue about this: https://github.com/guzzle/guzzle/issues/3114
- 🇮🇳India samit.310@gmail.com
Hi @handkerchief,
Agree with your point, but here the idea is the only trusted or some specific user roles have this permission Use the site in maintenance mode(
access site in maintenance mode
).The functionality will remain the same for other users, meaning whenever the maintenance mode will on all the users except those with the above permission will logout.
Thanks
Samit K. - 🇫🇷France zenimagine
hi, I found the website below which has a button to go up to the top of the page. It's great because when you scroll the page, it displays around the arrow the length of the page. Can you add an animation for the length of the page (a circle like on the website below):
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
The text only fail isn't going to admin/comment is what I'm saying
- 🇺🇸United States smustgrave
1) Drupal\Tests\system\FunctionalJavascript\OffCanvasTest::testOffCanvasNotResizable Failed asserting that a NULL is not empty. /builds/issue/drupal-3364302/core/modules/system/tests/src/FunctionalJavascript/OffCanvasTest.php:196 FAILURES!
Know there is that thread about the test but coverage is there.
I reverted the change
OffCanvasDialogTest
but then see a test failure so that's my mistake.Self applied a nitpicky :void return.
Believe this could be ready.
- 🇺🇸United States smustgrave
smustgrave → changed the visibility of the branch 3364302-allow-offcanvas-dialog to hidden.
- 🇺🇸United States smustgrave
Seems all the tags are still needed. Also forgot issue summary update from before
The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- 🇮🇹Italy mondrake 🇮🇹
I'd like to add more specific tests for the case of attempted rollback after a DDL statement is executed. On that.
- 🇺🇸United States smustgrave
@larowlan believe that is correct. Way I understand is that the discard button is taking you back to /entity_test/1/layout vs /admin/content
- 🇺🇸United States smustgrave
Only briefly skimmed but any change will need test coverage.
Also issue summary appears incomplete, recommend using default issue template. Sounds like from previous comment MR solution isn't along the idea so definitely needs a summary update.
- 🇺🇸United States smustgrave
Thanks for working on this
Tagging for issue summary as that appears to be incomplete, recommend using the standard issue template
Tagging for tests as feel a simple assertion could be added to an existing test.
- 🇺🇸United States smustgrave
Looking at the commit in https://git.drupalcode.org/project/drupal/-/commit/b1d41a52d4ac7905ef0b6... looks like these were missed.
- 🇺🇸United States smustgrave
smustgrave → changed the visibility of the branch 3463267-remove-lingering-priority to hidden.
- 🇺🇸United States smustgrave
Ignored error pattern #^Call to deprecated method getConfig\(\) of class GuzzleHttp\\Client\: Client\:\:getConfig will be removed in guzzlehttp/guzzle\:8\.0\.$# in path /builds/issue/drupal-3463251/core/tests/Drupal/Tests/Core/Http/ClientFactoryTest.php was not matched in reported errors. 58 Call to deprecated method getConfig() of interface GuzzleHttp\ClientInterface: ClientInterface::getConfig will be removed in guzzlehttp/guzzle:8.0. 🪪 method.deprecated ------ -----------------------------
Appears to have phpstan issues
@quietone that's a good question not sure we ever decided on that.
- 🇺🇸United States smustgrave
Ran test-only feature
1) Drupal\Tests\media\Functional\MediaOverviewPageTest::testMediaOverviewAuthorFilter Behat\Mink\Exception\ElementNotFoundException: Form field with id|name|label|value "name[media_field_data.user_name]" not found. /builds/issue/drupal-3113989/vendor/behat/mink/src/WebAssert.php:731 /builds/issue/drupal-3113989/core/tests/Drupal/Tests/UiHelperTrait.php:85 /builds/issue/drupal-3113989/core/modules/media/tests/src/Functional/MediaOverviewPageTest.php:219 FAILURES! Tests: 2, Assertions: 58, Failures: 1.
Manually testing editing the view I'm seeing the new filters.
Believe this is good
- @scott_euser opened merge request.
- First commit to issue fork.
- 🇳🇱Netherlands idebr
Added a separate branch for 11.x at https://git.drupalcode.org/project/drupal/-/merge_requests/8884
- @idebr opened merge request.
- First commit to issue fork.
- 🇬🇧United Kingdom longwave UK
There is a mismatch in the return value in the new method, and it made me realise the new method is doing two jobs; should we split it into two methods?
The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- 🇳🇿New Zealand quietone New Zealand
Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.
It is my understanding the the 'Needs Review Queue Initiative' tag is added after the issue has been reviewed by that initiative, so they can monitor the activity on reviewed issue. The definition → of this special tag supports my thinking. Am I wrong?
- First commit to issue fork.
- 🇮🇳India Kanchan Bhogade
Hi
I've tested MR 8872 on Drupal 11
The MR is applied cleanly...For no page scroll, there is No back-to-top button as expected, and when the page scrolls, the back-to-top button appears.
Attaching SS for reference
RTBC+1 - 🇳🇿New Zealand quietone New Zealand
Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.
- 🇳🇿New Zealand quietone New Zealand
Migrate Drupal and Migrate Drupal Ui are to be removed from core without replacement, this should not be needed.
- 🇬🇧United Kingdom alexpott 🇪🇺🌍
+1 to closing this as works as designed. Maybe some docs are missing. The fix in the MR would change settings.php for all test environments which would not be the correct fix here - but yeah there is no fix to do here :)
- 🇳🇿New Zealand quietone New Zealand
The release managers discussed this at the end of May and I am only now commenting. We noted that this is part of the Workflow Initiative but that initiative has ended and there isn't anything here for us to review. We also are aware that there is the Trash module and a core issue to move that to core, 📌 Add experimental Trash module Active .
This issue was also closed; as won't fix in 2017 and then re-opened in 2019. But since then there has been no activity here despite a prompt for more information 2 years ago. After 5 years without comment here from someone from the workflow initiative it seems sensible to restore the won't fix from 2017.
And, if that there is a desire to add this to core, then it would be better to start fresh with a new initiative or plan. Therefore, I am going to close this. I trust someone will correct that if it is wrong.
- 🇬🇧United Kingdom Matt B
I've switched to using the Entity PDF module, which uses mpdf as the engine. It is rendering Arabic characters fine.
- 🇮🇹Italy mondrake 🇮🇹
The SQLite test failure is unrelated; should be fixed by 🐛 GenerateThemeTest::testContribStarterkitDevSnapshotWithGitNotInstalled fails on sqlite Needs review .
- @idebr opened merge request.
- Issue created by @idebr
- 🇦🇲Armenia Murz Yerevan, Armenia
I made an MR https://git.drupalcode.org/project/drupal/-/merge_requests/8883 with implementing the
Drupal\Core\Http\ClientFactoryInterface
- please review. - 🇳🇿New Zealand quietone New Zealand
@mxr576, thanks for the snippet. It was very clear, thanks! I tweaked it a bit. There is an issue for the 10.4 beta release notes 📌 10.4.0-beta1 release notes Postponed . You may want to add that snippet to those sometime. I have updated the tags.
- @murz opened merge request.
- Issue created by @Murz
- 🇮🇳India Manthan.Chauhan
I have reroll patch no 126 as per drupal version 10.3.1.
- 🇦🇺Australia acbramley
This was triaged as part of Bug Smash Initiative.
I tried reproducing this on 10.3 but couldn't:
\Drupal::entityTypeManager()->getStorage('media_type') ->getQuery() ->condition('name', ['Document'], 'IN') ->accessCheck(FALSE)->execute();
I'm tempted to close this due to the lack of comments and updates since the last time this was traiged, but we will leave it in PMNMI.
Cheers.
- 🇳🇿New Zealand quietone New Zealand
I wasn't sure about this issue so I confirmed with @daffie that this is still relevant. In the same message they mentioned that there are other higher priority issues than removing these tests.
- 🇨🇦Canada AlexGreen
Couldn’t decide if it made more sense to throw an exception in
\Drupal\Core\Form\FormSubmitter::executeSubmitHandlers
or simply return early.If we throw an exception, then anything calling
FormBuilder::executeSubmitHandlers
will have to catch the exception, and if nothing catches it then the user will get a WSOD (i.e.: possibly from an expired form token because they left the page open in their browser for too long) which seems like a bad user experienceIf we return early, then anything calling
FormBuilder::executeSubmitHandlers
doesn’t really know that something went wrong (i.e.: the node didn't save)P.S. latest commit didn't include tests, was looking for feedback. (Test fails seem to be unrelated)
- 🇳🇿New Zealand quietone New Zealand
The patch here needs to be converted to an MR and allow the tests to run. I am all for tests but what is the test to do here? Is it just to confirm the refresh is done once? That needs clarification.
- 🇳🇿New Zealand DanielVeza Brisbane, AU
Rewrote the test to be FunctionalJS that follows the unhappy path
- 🇺🇸United States pbabin
I'm in Drupal 10.3.1 using Mercury Editor, Layout Paragraphs and Paragraphs.
We are getting the following error when editing a node with the above setup with a particular paragraph type which is referencing an image (the image is not using a view to render - it is default).
This entity (paragraph: [paragraph id #]) cannot be referenced.
I found initially that I could clone the entity, delete the original, and then save the node. However, on the next edit the issue still remained.
I tried the patch from 39 and could not apply it.
I tried the patch from 38 and it addressed my specific issue.
This change adds "layout--content-medium" to the element in the Olivero pager.html.twig file.
Not sure why it was added, but it messed up the layout of my site on a views page with pagination. I had to override the template and remove the class.https://git.drupalcode.org/project/drupal/-/commit/197d8bae7ddcab6093d30...
Leaving a comment here instead of opening a new ticket. Not sure if this is a bug, but some one else out there might be dealing with the same problem when updating to 10.3
- 🇺🇸United States smustgrave
I marked ✨ Add 'Is empty (NULL)' and 'Is not empty (NOT NULL)' to field filter operators Needs work as PNMI for someone to confirm this ticket fixes the same issue, if not that issue should specify what didn't make it.
For this ticket
Ran the test-only feature
1) Drupal\Tests\views\Kernel\Handler\FilterBooleanOperatorTest::testEmptyFilterBooleanOperator Failed asserting that actual size 6 matches expected size 1. /builds/issue/drupal-3322402/core/modules/views/tests/src/Kernel/Handler/FilterBooleanOperatorTest.php:161 FAILURES! Tests: 3, Assertions: 13, Failures: 1.
Show the test coverage.
Checking the issue summary, it appears complete with a matching solution to the MR.
Looking at the code I applied some small typehints for the new functions and the test.
I left
dataSet
andschemaDefinition
as those are inherited and probably could be a standalone ticket for applying throughout the repo.Rest appears to do as advertised.
- First commit to issue fork.
- 🇺🇸United States smustgrave
Can someone test if 📌 Add 'Is empty (NULL)' and 'Is not empty (NOT NULL)' operators to boolean field filtering Needs work solves the issue here? If not what should be addressed here that isn't addressed other there.
- 🇪🇸Spain rcodina
@smustgrave @alexpott Pipeline errors gone. Thanks for your tips!
- 🇺🇸United States benjifisher Boston area
I am adding a note to the issue summary about the patch for 10.2.x.
- 🇬🇧United Kingdom longwave UK
Note that 🐛 views.settings config object should not be used to cache list of available plugins Active proposes removing this config entirely, is effort better spent over there?
- 🇫🇷France mably
We are being hit by this rather annoying bug when using the views_auto_refresh → module.
Looks like a simple fix, it's surprising that it hasn't been merged yet.
Thanks for the patch!
- 🇺🇸United States smustgrave
Checked the threads from the old MR 3043 and they all seemed to have made it into the 11.x MR 8721
Moving these to a trait seems like a good idea. Not sure if it will need a CR though for contrib to use the new trait? But won't hold on that.
- 🇳🇱Netherlands bbrala Netherlands
Well at least everything is green. So thats good. But it some weirdness with the event subscriber. Not sure how that should work. I didn't investigate the redirects and such. Mostly just refactored the current recent patch to work with an event subscriber.
- 🇮🇳India amanmansuri72
Re-viewed the changes in the recent MR, Looks good.
Moving to RTBC+1
- 🇺🇸United States smustgrave
Removing CR tag as it appears to have been added, with example also
Ran test-only feature
1) Drupal\FunctionalJavascriptTests\Core\Form\JavascriptStatesTest::testJavascriptStates Failed asserting that true is false. /builds/issue/drupal-3010895/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php:355 /builds/issue/drupal-3010895/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php:69 FAILURES! Tests: 1, Assertions: 159, Failures: 1. Exiting with EXIT_CODE=1
So test coverage appears to be there.
Use of regex makes sense so believe this one to be good.
- 🇺🇸United States smustgrave
Added 1 super nitpicky void return directly.
Does appear #7 has been addressed and pipeline is green
Code wise use of array_filter makes sense and doesn't seem to cause any issue.
- 🇨🇭Switzerland handkerchief
@samit.310@gmail.com But not logging out does not automatically mean that users should continue to use the website despite maintenance.
Example from practice:
Website is briefly switched to maintenance mode several times a day because automatic backups are performed. However, it is correct that during backups, no user administers the website and therefore should not have the permission “Use the site in maintenance mode”. But the users should not all be logged out just because maintenance mode was switched on briefly. If you have a website with a community where many users are logged in, then they are automatically logged out several times a day. - 🇺🇸United States smustgrave
Think @alexpott answered this for you but there are now recipes in the repo with some image config that need the same fix you did for
core/modules/media_library/tests/modules/media_library_test/config/install/field.field.media.type_four.field_media_extra_image.yml - 🇳🇱Netherlands bbrala Netherlands
I played around with this and moved the implementation to a eventsubscriber as asked for by @znerol. I see one things that is very weird though as per comment in the code:
/** * Either Drupal\Tests\page_cache\Functional\PageCacheVaryTest::testPageCacheWithVary * fails or Drupal\Tests\language\Functional\LanguageBrowserDetectionAcceptLanguageTest::testAcceptLanguageEmptyDefault * fails if you remove one of those. Not sure why */ $events[KernelEvents::RESPONSE][] = ['onRespond', -100]; $events[KernelEvents::RESPONSE][] = ['onRespond', 100]; return $events;
Not sure why, those priorities sometimes confuse me a little.
- @bbrala opened merge request.
- 🇮🇳India samit.310@gmail.com
HI,
We have permission provided by Drupal Use the site in maintenance mode(
access site in maintenance mode
) that allow any user to work on maintenance mode, A simple change can fix this user logout issue,Here all the users who have Use the site in maintenance mode(
access site in maintenance mode
) permission, will not logout if the site goes to maintenance mode.Here is the PR link: https://git.drupalcode.org/project/drupal/-/merge_requests/8876/diffs
Thanks
Samit K. To maintain consistency some minor change done
Please review, moving NR
- @samit310gmailcom opened merge request.
- 🇮🇳India samit.310@gmail.com
samit.310@gmail.com → made their first commit to this issue’s fork.
- 🇳🇱Netherlands daffie
🐛 Ensure post transaction callbacks are always executed ONLY at the end of the root Drupal transaction Needs review has landed.
The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- 🇬🇧United Kingdom longwave UK
Decided to commit this to 11.1.x and 10.4.x only as there is a chance of it being vaguely disruptive to existing sites that are using RSS and not necessarily expecting CDATA in their output here, given we wrote a change record it makes sense to put it in a minor release only.
Still, great to see a 20+ year old bug finally fixed, and in such a clean way given we can just add the event subscriber to modify RSS responses.
Committed and pushed eba56b0fd7 to 11.x and ebc8e0639e to 10.4.x. Thanks!
-
longwave →
committed eba56b0f on 11.x
Issue #3433 by nicxvan, quietone, longwave, DuaelFr, smustgrave,...
-
longwave →
committed eba56b0f on 11.x
-
longwave →
committed ebc8e063 on 10.4.x
Issue #3433 by nicxvan, quietone, longwave, DuaelFr, smustgrave,...
-
longwave →
committed ebc8e063 on 10.4.x
- 🇮🇳India Hetal.Solanki
@all
I have reviewed MR !7436. It's looks good.
Moving to RTBC.
Thank you!! - 🇬🇧United Kingdom longwave UK
In the interests of not holding up a 20+ year old issue any longer, I fixed the nit and set it back to RTBC and will commit later if the tests pass.
- 🇬🇧United Kingdom longwave UK
I think this solution is really clean, just one nit about an unused argument.
Addressed test failures & pipeline passed successfully.
Please review, moving NR.
- 🇬🇧United Kingdom longwave UK
Fixing issue status and credits after crossposting with the bot.
-
longwave →
committed c29768f4 on 11.x
Issue #3451611 by mfb, xjm, smustgrave, quietone: Fix the format=flowed...
-
longwave →
committed c29768f4 on 11.x
-
longwave →
committed 1e5f032e on 11.0.x
Issue #3451611 by mfb, xjm, smustgrave, quietone: Fix the format=flowed...
-
longwave →
committed 1e5f032e on 11.0.x
-
longwave →
committed bc8de78f on 10.4.x
Issue #3451611 by mfb, xjm, smustgrave, quietone: Fix the format=flowed...
-
longwave →
committed bc8de78f on 10.4.x
- 🇬🇧United Kingdom longwave UK
Backported down to 10.3.x as a valid bug fix, although this is a very minor behaviour change it makes things better and I see no reason to keep it to major releases only.
Committed and pushed c29768f406 to 11.x and 1e5f032e26 to 11.0.x and bc8de78fe2 to 10.4.x and 5be98c43a8 to 10.3.x. Thanks!
-
longwave →
committed 5be98c43 on 10.3.x
Issue #3451611 by mfb, xjm, smustgrave, quietone: Fix the format=flowed...
-
longwave →
committed 5be98c43 on 10.3.x
The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- @ahsannazir opened merge request.
- 🇬🇧United Kingdom longwave UK
Opened 📌 Add helper method to generate HTML placeholders Active to simplify the code here.
Rebased MR with latest code & applied the left suggestion as well.
Please review, moving NR.
The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- 🇦🇲Armenia Murz Yerevan, Armenia
Seems we have another way to hide modules, intended only for tests - instead of using the parameter:
hidden: true
we can put all test modules into thetests
subdirectory of the module.
Then, they will be visible only in the test environment, and work well for Nightwatch tests too.Example: https://git.drupalcode.org/project/search_api/-/tree/8.x-1.x/tests/modul...
So, seems that marking test modules as hidden, is an outdated approach.
Therefore, instead of allowing hidden modules in tests, let's update the documentation at the page https://www.drupal.org/docs/develop/creating-modules/let-drupal-know-abo... → then? I updated the documentation now by adding:
For modules, intended only for tests, it's better to place them into the tests/modules subdirectory, instead of making them as hidden.
If it sounds okay for you, let's close the issue then?
- 🇦🇲Armenia Murz Yerevan, Armenia
That makes sense to me, do we have a use case for this today? if it helps existing tests we should convert at least one to showcase this.
There are no use cases for now because installing the hidden modules from Nightwatch tests is impossible.
So, for now, all of the tests that use hidden modules, are written with PHPUnit.I can write a new separate Nigtwatch test to reproduce and check this feature, but I think it will be just spending time on our pipelines.
Automatically closed - issue fixed for 2 weeks with no activity.
- 🇮🇳India ehsann_95
@finnsky, This makes sense and the above/inline field display is working as expected.
Inline:
Above:
- 🇮🇳India ehsann_95
The styles are looking as expected. Only thing i can figure out is the flex direction is column now. Attaching screenshot below
Automatically closed - issue fixed for 2 weeks with no activity.
- 🇺🇸United States smustgrave
Thanks @sleitner for updating the CR
@ronttizz your issue sounds like a block issue. Would recommend searching the issue queue to see if there is a similar ticket and leave a comment. Sometimes a simple comment of "saw this today on 10.3 while testing xyz" could bring people back to it. Joy of open source! haha
Feedback on the MR here appears to be address
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Committed to 11.x and backported to 11.0.x, 10.4.x and 10.3.x
Thanks folks
-
larowlan →
committed dc89b84c on 11.x
Issue #3454196 by james.williams, longwave: Filter placeholders without...
-
larowlan →
committed dc89b84c on 11.x
-
larowlan →
committed c1976a98 on 11.0.x
Issue #3454196 by james.williams, longwave: Filter placeholders without...
-
larowlan →
committed c1976a98 on 11.0.x
-
larowlan →
committed f3d8334b on 10.4.x
Issue #3454196 by james.williams, longwave: Filter placeholders without...
-
larowlan →
committed f3d8334b on 10.4.x
-
larowlan →
committed 883e240e on 10.3.x
Issue #3454196 by james.williams, longwave: Filter placeholders without...
-
larowlan →
committed 883e240e on 10.3.x
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Left some questions on the MR, feel free to self-RTBC after replying/changes
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Committed to 11.x and backported to 11.0.x, 10.4.x and 10.3.x
-
larowlan →
committed cc32980f on 11.x
Issue #3392572 by benjifisher, Liam Morland, ricovandevin, Anybody,...
-
larowlan →
committed cc32980f on 11.x
-
larowlan →
committed 8418cba1 on 11.0.x
Issue #3392572 by benjifisher, Liam Morland, ricovandevin, Anybody,...
-
larowlan →
committed 8418cba1 on 11.0.x
-
larowlan →
committed 7599ccbd on 10.4.x
Issue #3392572 by benjifisher, Liam Morland, ricovandevin, Anybody,...
-
larowlan →
committed 7599ccbd on 10.4.x
-
larowlan →
committed c082ee7e on 10.3.x
Issue #3392572 by benjifisher, Liam Morland, ricovandevin, Anybody,...
-
larowlan →
committed c082ee7e on 10.3.x
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Committed to 11.x
Per https://www.drupal.org/about/core/policies/core-change-policies/allowed-... → this isn't eligible for backport to 11.0.x.
Glad to see this one resolved.
-
larowlan →
committed 6a685718 on 11.x
Issue #2329253 by tstoeckler, Bhanu951, hchonov, golddragon007,...
-
larowlan →
committed 6a685718 on 11.x
- 🇫🇷France nod_ Lille
That makes sense to me, do we have a use case for this today? if it helps existing tests we should convert at least one to showcase this.
- 🇺🇸United States smustgrave
Re-ran failure for the editor module and appeared to be random.
Believe all feedback for this one has been addressed.
- 🇺🇸United States smustgrave
Perfect test-only job has already been ran https://git.drupalcode.org/issue/drupal-3416525/-/jobs/2118936
Thanks also for adding the steps and summary update. Confirmed solution matches what's in the MR.
Code wise the early return looks fine.
Believe this may be good.
- 🇺🇸United States smustgrave
The addition to subtraction is equal so confident in the use of :void.
I spot checked a few dozen and change appears to be as advertised.
Didn't run a grep but this seems like a good chunk and any stragglers I think could be handled later if needed.
- 🇺🇸United States gtucker6
I hate to be the bearer of bad news on such a long sought after solution for this, but the patches above do not work for 10.3 (I can't verify for Drupal 9 and below). This is also not an ideal nor practical solution. The getBlockFromAjaxRequest() method that was added does not return the expected results. The block gets a duplicate config key and also returns null on the first ajax request via elementPreRender(). The preBlockBuild() function does not get called still because the ajax architecture only returns the view (the block is a wrapper around the view and does not get updated).
My solution for the client that needed this was as follows:
1. Add Drupal JS settings for views blocks: $build['#attached']['drupalSettings']['viewsBlock'][$this->view->dom_id] = $block->getConfiguration();
2. Add a new method to the views block display and views block plugin to add the settings to the build: alterBlockBuild(ViewsBlock $block, &$build)
3. Add an event subscriber with a that responds to the ViewAjaxResponse response and checks for the block display handler
4. Add an ajax command that responds on ajax and uses the block settings for updating the viewI really think our proposed solution should be changed to something similar to above. This way is much cleaner and follows the ajax workflow mostly with views. Any module that overrides views block settings could then implement their own subscriber and ajax commands with the new viewsBlock drupal settings in the block build.
I spent over 25 hours on this issue, and the only way I could get block settings to update the view, was via drupalSettings on the views block and an ajax command that fires on the response of ViewsAjaxResponse.
The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- 🇺🇸United States smustgrave
Makes 100% sense to me to be able to install test modules.
- 🇷🇸Serbia finnsky
I started a new approach because I believe that styles in components should not be tied to Drupal. This makes them more flexible.
So it shouldn't be
.field .node-- and other thingsJust pure simple and clear css
Also fixed flexboxes and added support for label above
Please check!
- @finnsky opened merge request.
- 🇷🇸Serbia finnsky
I lile this new approach more than previous.
Now i've used `span 2 or 3`
so 25 + 25 + 50 has same middle gap as 50 + 25 + 50
etc.Please review!
- @finnsky opened merge request.
- 🇳🇱Netherlands Summit
Hi,
Can I use this patch on Drupal 10.3.1? https://www.drupal.org/files/issues/2024-06-28/3163197-32.patch →Thanks for building it! Greetings,
- 🇪🇸Spain ckrina Barcelona
First, thanks all for working on this and finding a place that needs improvement. I agree with you all on this being one of those places.
We have restored the warning icon before the deprecated text on deprecated modules, just like in the Seven theme.
Even if this existed in Seven, it doesn't mean it'll visually work in Claro: it results in a really packed, confusing and un-structured group of elements. The solution proposed here is not the right one on a design/UI perspective. The title/heading is designed to hold the title, not title + parenthesis and its text + icons. We can't add info and elements to places that are not designed for that when there are no designs.
The goal of this issue is to make more obvious that a module is obsolete. +1 to that. The next step here is to find the right UI solution for Claro (applies to any UI), and that needs a designer designing that new part of the UI that don't exists (even if that existed in Seven, that solution doesn't work here). When that design is aproved, then let's jump into the implementation. This is the best workflow to be sure we end up with digestible UIs. :)
- 🇪🇸Spain rcodina
@smustgrave Could you please check out current pipeline error? It doesn't make sense to me.
@alexpott All threads resolved.
- 🇳🇿New Zealand quietone New Zealand
If I read this correctly, in #8 @jhodgdon said there were no changes to be made on this page. Then, in #28 said that this could be changed to adjusting URLs here and elsewhere. But, URLs have been updated in 🐛 [META] Many documentation / handbook URLs redirect to D7 content Needs work and [##3248078]. So, as far as I can tell, there is nothing to do here. I am closing as outdated. If that is wrong add an explanatory comment and set the status to 'active'.
- 🇳🇿New Zealand quietone New Zealand
We should explain where they need to go for core lib tests too -- core should be documenting itself!
Let's move that to a follow up issue so we can make this improvement.
I have convert to an MR and made a slight change to indicate this is for modules.
- @quietone opened merge request.
- First commit to issue fork.
The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- @quietone opened merge request.
- First commit to issue fork.