- 🇳🇿New Zealand quietone
Thanks!
Committed to 11.x, 11.0.x, 10.4.x, and 10.3.x.
-
quietone →
committed 833e599d on 11.x
Issue #2479449 by sudiptadas19, smustgrave, akashkumar07, rithesh bk,...
-
quietone →
committed 833e599d on 11.x
-
quietone →
committed 0a6b5691 on 11.0.x
Issue #2479449 by sudiptadas19, smustgrave, akashkumar07, rithesh bk,...
-
quietone →
committed 0a6b5691 on 11.0.x
-
quietone →
committed 7bf99407 on 10.4.x
Issue #2479449 by sudiptadas19, smustgrave, akashkumar07, rithesh bk,...
-
quietone →
committed 7bf99407 on 10.4.x
-
quietone →
committed 57ef3662 on 10.3.x
Issue #2479449 by sudiptadas19, smustgrave, akashkumar07, rithesh bk,...
-
quietone →
committed 57ef3662 on 10.3.x
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Requeued tests, added question about another possible enhancement we could add here
- 🇪🇸Spain fjgarlin
isMaintained
is just a boolean that says whether the maintenance status of a module is within those 3 posted above.
Code:isMaintained: in_array($maintenance_status['id'], self::MAINTAINED_VALUES),
The filters work independently from how that property is calculated.
- 🇪🇸Spain fjgarlin
I see field_maintenace_status here: https://www.drupal.org/jsonapi/index/project_modules?filter%5Bmachine_na... →
and a query using the filter here →// 'Actively maintained'. '49125cb6-2f35-451b-922d-3042cb1b4391', // 'Minimally maintained' 'de457d5a-2ce3-45b4-b88b-1c54e5e6d0e2', // 'Seeking co-maintainer(s) 'fd8b539f-a5e4-4577-9367-f119a252327b',
- 🇺🇸United States chrisfromredfin Portland, Maine
We definitely want to keep URL - we're using that on the detail page now. What I'm amazed by is that getting rid of isMaintained here still lets the "maintained" filter work on the front-end. Why is that?! Do we not need that in the backend object because the front-end doesn't directly use the backend object? Or does the JSON:API endpoint just ignore filters provided that don't matter?
I can't seem to find an example where an unmaintained module shows with manual testing. Can anyone help?
The first order of business here is for someone to digest and understand the interplay between the unused project properties we're removing from the PHP objects, and how those are then used on the frontend. Me, without that knowledge, cannot review this and know if getting rid of these properties "works."
But I can say we need isMaintained and URL on the front-end, for sure.
- @maneesha-binish opened merge request.
- 🇬🇧United Kingdom nexusnovaz
Another issue with phpstan. has less information than #61
---------------------------------------------------------------------------------------------------- Running PHPStan on changed files. PHPStan: failed ----------------------------------------------------------------------------------------------------
I am unsure how to fix this
- 🇧🇪Belgium borisson_ Mechelen, 🇧🇪
I think we can still work on this issue, or if we move to another issue we need to credit the people who helped already here. I think it's a safe move to reuse this issue.
- 🇬🇧United Kingdom jonathan1055
Thank You Klausi for your understanding and also your commitment to getting Coder changes implemented.
For those coming here having had this problem, if you want to override the case-sensitivity setting, the the following to your project's phpcs.xml file:
<rule ref="SlevomatCodingStandard.Namespaces.AlphabeticallySortedUses"> <properties> <property name="caseSensitive" value="false"/> </properties> </rule>
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.
- 🇩🇪Germany Anybody Porta Westfalica
Thank you @vladimiraus - Tests are green again, also for other issues!
- 🇳🇿New Zealand quietone
I read the issue, comments and the MR etc. I found all questions answered and all core gates have been addressed. I think this is good to go.
I have updated credit.
Assigning to myself to commit within 24 hours.
Some change upstream must have affected the PHPStan check, because the bot failed on this:
---------------------------------------------------------------------------------------------------- Running PHPStan on changed files. [ERROR] No files found to analyse. PHPStan: failed ----------------------------------------------------------------------------------------------------
-
vladimiraus →
committed 6efe2168 on 8.x-3.x authored by
anybody →
Issue #3481730: Fix phpcs, cspell and phpunit
-
vladimiraus →
committed 6efe2168 on 8.x-3.x authored by
anybody →
- 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.
- First commit to issue fork.
- 🇬🇧United Kingdom nexusnovaz
An unrealted test was failing, but after a rerun, its all green now. Marking as needs review unless i've missed something else?
- 🇩🇪Germany Anybody Porta Westfalica
Thank you for your feedback and input @rhovland! Learned something new today! Thanks!!
- 🇺🇸United States rhovland Oregon
When addressing spelling issues adding words to the dictionary should only happen for valid words.
For instance "fewfsfs" is part of an API key. This should not be added to the dictionary. Instead the the following should be appended to the line above the API key in the test
// cspell:disable-next-line
The address in the test "2630 Hegal Place" should be modified so it does not trip the spell checker.
The usage of "autofills" is incorrect. It should say "Chrome autofill incorrectly fills this field with address line 1" as that is clearer language.
Patch #50 🐛 Creating `Group` content is impossible via REST, JSON:API and GraphQL due to `$context['group']` being required in checking `create` access Needs work fails on group 3.3.0 due to a trivial mismatch in the imports. I've re-rolled it as patch #54 → .
- 🇪🇸Spain facine
Fixed, thank you!
Please download and review the latest dev release.
- @facine opened merge request.
- @facine 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.
Automatically closed - issue fixed for 2 weeks with no activity.
- 🇮🇳India atul_ghate
Hello @clarkssquared,
Could you please elaborate on what exactly is wrong according to the Drupal README standards? This will help me to make the necessary modifications more easily. Thank you! - 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
The changes to the link templates here look good, but I'm concerned about the impact on contrib modules that are using these entity types for their tests. This can be seen by the large number of changes in the MR to add the additional type to the URL.
Can we try to revert all of those changes, and as well as fixing the add-form template, add an add-page template (using the original URL).
For instances where there's only one bundle, that will wire up
\Drupal\Core\Entity\Controller\EntityController::addPage
which handily redirects to the add-form template when there is only one bundle. For the vast majority of tests, there will only be one bundle because of the default value in\entity_test_entity_bundle_info
We can then assess how many tests are still broken, but I suspect it will be dramatically less. That will give us confidence that this change won't break as many contrib tests as the current approach most likely will.
- 🇨🇦Canada Chris_Tomasso
Hello,
I've removed an unnecessary @trigger_error that I didn't believe was required.
I hope this change aligns with your expectations.You can review the changes here: Merge Request #6902.
Additionally, I’ve included the test and change record as requested by smustgrave.
Let me know if there's anything.
- 🇺🇸United States smustgrave
1) Drupal\Tests\Core\Cache\ChainedFastBackendTest::testSetInvalidDataFastBackend Invalid data was not saved on the fast cache. stdClass Object (...) does not match expected type "NULL". /builds/issue/drupal-3395212/core/tests/Drupal/Tests/Core/Cache/ChainedFastBackendTest.php:103 FAILURES! Tests: 3, Assertions: 7, Failures: 1. Exiting with EXIT_CODE=1
Shows test coverage.
Not sure if this needs any kind of release note (assuming not)
Looking at the change to core/lib/Drupal/Core/Cache/ChainedFastBackend.php and don't see anything glaringly wrong. Think adding the condition makes sense.
believe this one is good.
- 🇺🇸United States smustgrave
Failure seems unrelated, re-running. Resolved the threads while I was at it. Bot feedback appears addressed.
- 🇺🇸United States smustgrave
Hiding old patches and files
Issue summary appears complete so removing that tag.
Ran test-only feature and it failed but didn't output a failure so ran locally
Behat\Mink\Exception\ResponseTextException: The text "Password field is required." was not found anywhere in the text of the current page. /var/www/html/web/vendor/behat/mink/src/WebAssert.php:907 /var/www/html/web/vendor/behat/mink/src/WebAssert.php:293 /var/www/html/web/core/tests/Drupal/Tests/WebAssert.php:979 /var/www/html/web/core/modules/user/tests/src/Functional/UserPasswordResetTest.php:143
Looking at the change to core/modules/user/src/AccountForm.php and comment helps and makes sense to me.
believe this is good.
- 🇺🇸United States smustgrave
Seems the bot missed some return types also.
But issue summary appears to be incomplete, should be following standard template.
- 🇺🇸United States traviscarden
@secretsayan asked me to push this change for him real quick because he's in the process of moving to a new laptop.
- 🇺🇸United States smustgrave
Please look at the tags before putting into review. Appears to be a reroll
- 🇬🇧United Kingdom catch
Committed/pushed to 11.x, thanks!
Did my best with issue credit but there was a lot of issue history for a three line MR!
- 🇬🇧United Kingdom jessebaker
Reviewed, but further changes/refactor needed, thank you.
- 🇬🇧United Kingdom scott_euser
I know these issues are ancient, but I was trying to browse through the user interface related issues to see if there are any plans in place in general. We started leveraging Search API for the 'AI Search' sub-module of AI and have extended some form classes. Would be interested in contributing to modernising the 'Add fields' wizard/edit approaches to match Drupal Core more (also so things like Gin/Claro/etc all adapt nicely). Is this the best issue still to work from? Or start fresh with new issue and "Closed (outdated)" some of these old ones?
- 🇬🇧United Kingdom longwave UK
I have restarted those tests as the fails look unrelated. The fix itself looks correct, so moving to RTBC.
- @atul_ghate opened merge request.
- 🇵🇭Philippines jan.fetalvero
jan.fetalvero → changed the visibility of the branch 3481418-typo-in-error to active.
- 🇵🇭Philippines jan.fetalvero
jan.fetalvero → changed the visibility of the branch 3481418-typo-in-error to hidden.
- Issue created by @joachim
- 🇬🇧United Kingdom joachim
> Which sounds like undefined severities default to REQUIREMENT_OK while installing, and REQUIREMENT_INFO otherwise.
Yup, well spotted!
(Also, the logic in that piece of code could be a lot clearer - I'll file an issue to refactor it.)
(Also, it seems like REQUIREMENT_INFO might only apply during runtime? Again, something for a separate issue.)
- 🇵🇭Philippines clarkssquared
Hi @atulghate
I checked the README.md file but it seems its not organized according to Drupal README.md template →
Re #37 🐛 hook_requirements() doesn't say that severity is optional, or what the default is RTBC :
Actually, I am mistaken. A change is needed here. The default value is REQUIREMENT_INFO, see
#665790-291: Redesign the status report page
https://git.drupalcode.org/project/drupal/-/blame/11.x/core/modules/syst...The linked code still matches what was mentioned in #2 🐛 hook_requirements() doesn't say that severity is optional, or what the default is RTBC , and should probably be documented as such:
$severity = $severities[REQUIREMENT_INFO]; if (isset($requirement['severity'])) { $severity = $severities[(int) $requirement['severity']]; } elseif (defined('MAINTENANCE_MODE') && MAINTENANCE_MODE == 'install') { $severity = $severities[REQUIREMENT_OK]; }
Which sounds like undefined severities default to REQUIREMENT_OK while installing, and REQUIREMENT_INFO otherwise.
This should indeed be documented explicitly.
- 🇸🇬Singapore anish.a Singapore
There are test failures. I am unsure about how to fix it.
Automatically closed - issue fixed for 2 weeks with no activity.
-
smustgrave →
committed c11597b4 on 2.0.x
Issue #3481865 by penyaskito: Typo in composer suggestion:...
-
smustgrave →
committed c11597b4 on 2.0.x
- Issue created by @penyaskito
Hello, good afternoon, here I leave you a complete alternative for your README.md file
- 🇨🇴Colombia kinyein
Hi, I reviewed this and it looks good, I tried to apply all the patches but the first didn't work for me, but still looks good.
You could try something like this:
# Date Popup## Introduction
The **Date Popup** module enhances all date fields in views filters by adding the native HTML5 date picker widget, making it easier to select and filter dates with an intuitive popup interface.
## Table of Contents
1. [Requirements](#requirements)
2. [Installation](#installation)
3. [Configuration](#configuration)## Requirements
This module does not require any additional modules outside of Drupal core.
## Installation
To install the **Date Popup** module, follow these steps:
1. **Using Composer**
If your Drupal site is managed via Composer, you can install the module by running the following command:
```bash
composer require "drupal/date_popup"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.
- 🇮🇳India akulsaxena
Hi @pfrenssen
Thank you for the credit
It was my first credit. Means a lot.
I'll keep the mistakes i made in mind so that i can do even better from next time. - 🇧🇬Bulgaria pfrenssen Sofia
Thanks for the work so far! I did an initial review, I found a few small issues.
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.
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.
- 🇮🇳India lavanyatalwar
Thanks @ankitv18. I agree, I could have used that method.
- 🇩🇪Germany Anybody Porta Westfalica
Here we go. Drupal now adds more classes on the draggable, that was causing the test to fail. No error in the module itself!
- @anybody opened merge request.
- Issue created by @Anybody
- 🇮🇳India sagarmohite0031
Hello rachel_norfolk,
I have reproduced the issue for Spacing issue between Checkbox label and button on Drupal 11 for Claro.
The MR is applied successfully.Steps to reproduce
Step1 : Fresh install Drupal 11 and use default Claro theme.
Step2 : Enable layout builder module
Step3 : Use Layout Builder from Administration > Structure > Content types > Basic page > Manage display > default > layout options
Step4 : Create Basic page from Content > Add content > Basic page and Save
Step5 : Click on Layout tab and you can find the spacing issue between Checkbox label and button.See attached screen shot for reference.
Test Result:
Spacing issue between Checkbox label and button has been fixed.
Check attachments. - 🇮🇳India ankitv18
@lavanyatalwar we can use _CSPELL_WORDS in case of few words are reported and you create .cspell-project-words.txt file to add all words as a dictionary in that file.
Please proceed with other feedback from @anybody
- First commit to issue fork.
-
pfrenssen →
committed 1005680b on 3.0.x authored by
akulsaxena →
Issue #3480495 by akulsaxena, pfrenssen: Adopt strict typing
-
pfrenssen →
committed 1005680b on 3.0.x authored by
akulsaxena →
- 🇧🇬Bulgaria pfrenssen Sofia
You can always move an issue to Needs Review if you want someone to review it. But I am taking care of it now. I fixed the type bugs and am waiting for the test result. If this comes back green this is good to go and I will merge it. I'm pretty sure a bunch of typing bugs that are not yet covered by tests will surface very quickly, but these can be handled in followups.
Thanks very much for the contribution!
- 🇮🇳India akulsaxena
Thanks @pfrenssen,
Should I move this strict type declaration issue to needs review now that the tasks mentioned in this issue are complete or let it stay in needs work?
- 🇧🇬Bulgaria pfrenssen Sofia
This is looking very good already. Thanks also for the documentation updates.
The goal of using strict types is to find typing bugs, so these failures are showing that we have some bugs that should be fixed. There are undoubtedly more bugs, which will start to surface once this is merged. For now I will tackle the ones that are found.
- 🇮🇳India lavanyatalwar
Thanks @anybody
Sure I'll have a look on the issue you are refering.
Let me first resolve the comments on this issue. - 🇩🇪Germany Anybody Porta Westfalica
Really really GREAT work @lavanyatalwar thank you so much!
I left some comments to resolve. Would you also like to have a look at the fix tests issue afterwards?
- 🇮🇳India divyansh.gupta Jaipur
I removed the test failure which was due to mismatch of create function with its parent class.
Please review it and tell me if any changes are to be made - 🇮🇳India akulsaxena
Hey @pfrenssen
So I was trying to cast the variables into int to remove the errors in phpUnit pipeline.
Please note, that these errors are not related to the changes I made in the branch. All I changed was added strict type declarations and a custom file to add ruleset as mentioned in the issue. But when i try to solve the phpUnit pipeline error, i cast the variable given into int and then run the pipeline again and then it gives me same casting error for another variable.
Like for example - Similiarly in file MonthlyRecurringDateTest.php, the method testFindMonthDaysBetweenDates expects the $day argument to be an int, but a string is being passed. when i casted it into int and ran the pipeline again, it gave me the same error for $year, and then for $month when i ran the pipeline again and then for $timestamp. So what it is doing is giving me one error at a time and then when i solve that and commit and then push and run the pipeline again, thats when it gives me another error. I dont know how many variables need casting and at how many places as I did not write the code and dont know what has been done in each file and solving this typecasting error one by one doesnt seem the right approach.
Please have a look - 🇮🇳India lavanyatalwar
@anybody
Now all the errors have been resolved.
Kindly check and merge :) Rebased this to the latest 2.0.x.There are a couple of issues that @fjgarlin pointed related to
isMaintained and URL
so wanted a consensus on what needs to be done.I'll again mark this NR just to double confirm what are next steps on this and maybe ping in the slack channel .- 🇮🇳India lavanyatalwar
Okay
Should I fix phpunit errors as well?
Because I see some errors related to views as well (missing schema) that is not related to the this module. - First commit to issue fork.
- 🇩🇪Germany Anybody Porta Westfalica
@lavanyatalwar thanks. For "names" you could add a cspell ignore file to add these special words, like the module name.
If it's not the module name, it should be spaced:
Giftcard => Gift card
Giftcards => Gift cards - 🇮🇳India lavanyatalwar
Heyy @anybody,
I was resolving cspell errors and found the problem lies mostly in the term 'giftcards'.
Pipeline failure error log suggests to change it to 'giftcard' instead.
I think changing it will generate an error in th functionality.
So should I skip cspell or change the term? - 🇺🇦Ukraine Ruslan Piskarov Kiev, Ukraine
+1 for an official release. Thanks for working on this!
- 🇮🇳India nayana_mvr
Marking it as 'Needs Review' to get feedback from maintainer.
- 🇮🇳India nayana_mvr
Verified this issue in Drupal 10.4.x as Entity embed module is not compatible with D11. The css code added in #2 patch has been already implemented for the Claro (core admin theme) so this issue is not reproducible with Claro theme (current core admin theme). As for Olivero (core default theme), the dropdown is still clipped with a scroll in the dialog but I think it is fine because in Olivero 'Next' button is coming in the left bottom. If dropdown is not clipped, it will overlap on the 'Next' button. I'm not sure whether we need to make any changes at system level now since already theme level fix is implemented for Claro. Attaching screenshots for reference.
Claro:
Olivero:
Automatically closed - issue fixed for 2 weeks with no activity.
Automatically closed - issue fixed for 2 weeks with no activity.
- @anybody opened merge request.
- Issue created by @Anybody
- 🇮🇳India akulsaxena
Hey @pfrenssen
In this issue you have provided the branch recurring_events-3480495. When i ran phpcs in it, it showed a few files missing the file doc comment and only 3 files missing the strict_type declarations. I think the rest of the files that are missing the declarations are in the parent branch recurring_events.
Also, phpcs gives a recommendation to change the permission from "access administration pages" to "administer site configuration" in line 198 in the recurring_events.routing.yml file. Should I let the permission as it is, or should I change it as to what phpcs suggests.
The errors of phpunit also exist in the parent module (recurring_events) and not in the fork given in this issue. - 🇮🇳India akulsaxena
Thanks for the help
I'll work on the errors and solve them soon - @miguelaguero opened merge request.
- 🇧🇬Bulgaria pfrenssen Sofia
The composer build works again! The custom PHP CodeSniffer file also is working. It looks like 16 files are still missing the strict types declaration. And already a couple type bugs are detected in the PHPUnit tests :) That's great and the whole point of doing this.
Setting back to needs work, the remaining strict_type declarations need to be added, and the type bugs fixed so the tests are green again.
Thanks a lot for the work so far!
- 🇦🇹Austria klausi 🇦🇹 Vienna
Very sorry for the disruption this caused.
The coding standards issue in question has been worked on for 12 years, I don't want to wait that long in Coder for adopting useful standards.
Now it turns out that the case-sensitivity of the standard causes pain - probably best if we open a new issue to weigh pros and cons of a revert.
In the meantime please use workarounds:
* Disable this sniff in your project
* Configure phpcbf in your Phpstorm to auto-fix the use statement ordering so that you don't have to think about it. - @miguelaguero opened merge request.
- First commit to issue fork.
-
anybody →
committed 1659a8d0 on 3.x authored by
kul.pratap →
Issue #3481442 by kul.pratap, anybody: Fix PHPCS & cspell
-
anybody →
committed 1659a8d0 on 3.x authored by
kul.pratap →
- 🇩🇪Germany Anybody Porta Westfalica
Whao, thank you @kul.pratap! Great work!! RTBC
- 🇧🇬Bulgaria pfrenssen Sofia
It's because of the unrelated changes to the composer files, these files shouldn't be changed.
- 🇮🇳India kulpratap2002
@anybody i have solved all the phpcs and cspell errors but it is failing the phpunit in pipeline, should i also solve that?
- 🇨🇦Canada SKAUGHT
As per #50 and #48. I'll move to RTBC logo is aligned with the rest of items below, matching overall design mocks.
#48 notes. " But there should be some space between logo and slider." I would agree the width of the entire item might ought to be wider but that scope that might pushing this ticket IMO. Also, as we know various browsers use different widths.
- @kulpratap opened merge request.
- 🇺🇸United States smustgrave
Removing novice tag appears to be more advanced then that
Was also tagged for change record which sitll needs to be needed. Automatically closed - issue fixed for 2 weeks with no activity.
Automatically closed - issue fixed for 2 weeks with no activity.
- 🇮🇳India akulsaxena
Hi @pfrenssen
I made the necessary changes rebased the commits to 3.0.x branch. the pipeline issue still shows up. Please have a look - Issue created by @Anybody
- 🇮🇳India divyansh.gupta Jaipur
ok I will make the changes accordingly and make the test pass.
- 🇧🇬Bulgaria pfrenssen Sofia
Awesome, thanks so much for working on this. I see there is a test failure, can you have a look? It looks like the introduction of the constructor in
WeeklyRecurringDateWidget
needs to be adapted to match the parent class. - 🇧🇬Bulgaria pfrenssen Sofia
Thanks for working on this! The MR has been made against the 2.x branch, but it should be done against 3.0.x instead. Can you rebase it? If the error still persists I can have a look. Normally no changes are needed in composer files.
- 🇬🇧United Kingdom tomdearden
Looks like the MR fails testing but not (as far as I can tell) on anything I did! I'm going to have to leave this to other, cleverer people than me to address, I think.
- 🇬🇧United Kingdom tomdearden
MR raised with a fix:
https://git.drupalcode.org/project/drupal/-/merge_requests/9862
- @tomdearden opened merge request.
- Issue created by @tomdearden
- 🇬🇧United Kingdom joachim
Thanks for the MR! Though remember to change the status to Needs Review when you want it looked at.
I've left comments on the MR for changes.
- 🇮🇳India arunkumark Coimbatore
I faced the issue of creating MR against the 11.x branch. Now it is working to update the MR
- 🇮🇳India arunkumark Coimbatore
arunkumark → changed the visibility of the branch 2982582-hookrequirements-doesnt-say-d11 to hidden.
- 🇮🇳India arunkumark Coimbatore
arunkumark → changed the visibility of the branch 2982582-hookrequirements-doesnt-say to active.
- 🇮🇳India nayana_mvr
Verified the changes in MR!8893 in Drupal version 11.x and can confirm that it fixes the issue. The position remained centre aligned even while scrolling the navigation bar (please refer the screen recording). Attaching before and after screenshots as well. Need RTBC+1
Before:
After:
Updated IS with latest screenshots.
- 🇮🇳India akulsaxena
Hi @pfrenssen
I made the necessary changes and Included declare(strict_types=1); at the top of every PHP file.
I also Updated the coding standards to require strict types by creating a phpcs.xml.dist file
But when i am creating an MR, the pipeline is failing at composer buildThis is the error that shows up:
Running with gitlab-runner 17.3.1 (66269445) on gitlab-runner-9dccd6855-vtfgl s8ex1X2yJ, system ID: r_Fs6duF1rd5sK Resolving secrets Preparing the "kubernetes" executor 00:00 "CPURequest" overwritten with "2" Using Kubernetes namespace: gitlab-runner Using Kubernetes executor with image drupalci/php-8.3-apache:production ... Using attach strategy to execute scripts... Preparing environment 00:31 Using FF_USE_POD_ACTIVE_DEADLINE_SECONDS, the Pod activeDeadlineSeconds will be set to the job timeout: 1h0m0s... Waiting for pod gitlab-runner/runner-s8ex1x2yj-project-162884-concurrent-0-g2pu6aqs to be running, status is Pending Running on runner-s8ex1x2yj-project-162884-concurrent-0-g2pu6aqs via gitlab-runner-9dccd6855-vtfgl... Getting source from Git repository 00:01 Fetching changes... Initialized empty Git repository in /builds/issue/recurring_events-3480495/.git/ Created fresh repository. Checking out 8a88c2b2 as detached HEAD (ref is 3480495-adopt-strict-typing)... Skipping Git submodules setup Executing "step_script" stage of the job script 00:01 $ composer --version Composer version 2.8.0 2024-10-02 16:40:29 PHP version 8.3.12 (/usr/local/bin/php) Run the "diagnose" command to get more detailed diagnostics output. $ # Forks are named "module_name-12345", so strip out the number part to get the correct module name. # collapsed multi-line command $ if [ "$_SHOW_ENVIRONMENT_VARIABLES" == "1" ]; then # collapsed multi-line command $ if [ "$_LENIENT_ALLOW_LIST" != "" ]; then # collapsed multi-line command $ [[ $_CURL_TEMPLATES_REF == "" ]] && export _CURL_TEMPLATES_REF=$_GITLAB_TEMPLATES_REF # collapsed multi-line command $ echo "Executing curl -OL https://git.drupalcode.org/$_CURL_TEMPLATES_REPO/-/raw/$_CURL_TEMPLATES_REF/scripts/expand_composer_json.php" Executing curl -OL https://git.drupalcode.org/project/gitlab_templates/-/raw/default-ref/scripts/expand_composer_json.php $ curl -OL https://git.drupalcode.org/$_CURL_TEMPLATES_REPO/-/raw/$_CURL_TEMPLATES_REF/scripts/expand_composer_json.php % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 100 8538 100 8538 0 0 69577 0 --:--:-- --:--:-- --:--:-- 69983 $ if [[ -f composer.json ]]; then # collapsed multi-line command $ php expand_composer_json.php || EXPAND_COMPOSER_EXIT_CODE=$? drupalConstraint = 10.3.6 core_version=10.3.6 Writing output to composer.json $ if [[ "$EXPAND_COMPOSER_EXIT_CODE" != "" ]]; then echo "EXPAND_COMPOSER_EXIT_CODE=$EXPAND_COMPOSER_EXIT_CODE"; exit $EXPAND_COMPOSER_EXIT_CODE; fi $ composer install $COMPOSER_EXTRA Installing dependencies from lock file (including require-dev) Verifying lock file contents can be installed on current platform. Warning: The lock file is not up to date with the latest changes in composer.json. You may be getting outdated dependencies. It is recommended that you run `composer update` or `composer update <package name>`. - Required (in require-dev) package "composer/installers" is not present in the lock file. - Required (in require-dev) package "drupal/core-composer-scaffold" is not present in the lock file. - Required (in require-dev) package "cweagans/composer-patches" is not present in the lock file. - Required (in require-dev) package "drupal/core-recommended" is not present in the lock file. - Required (in require-dev) package "drupal/core-dev" is not present in the lock file. - Required (in require-dev) package "php-parallel-lint/php-parallel-lint" is not present in the lock file. This usually happens when composer files are incorrectly merged or the composer.json file is manually edited. Read more about correctly resolving merge conflicts https://getcomposer.org/doc/articles/resolving-merge-conflicts.md and prefer using the "require" command over editing the composer.json file directly https://getcomposer.org/doc/03-cli.md#require-r Uploading artifacts for failed job 00:02 Uploading artifacts... .: found 373 matching artifact files and directories .git: excluded 1 files .git/**/*: excluded 71 files Uploading artifacts as "archive" to coordinator... 201 Created id=3074846 responseStatus=201 Created token=glcbt-64 Uploading artifacts... build.env: found 1 matching artifact files and directories Uploading artifacts as "dotenv" to coordinator... 201 Created id=3074846 responseStatus=201 Created token=glcbt-64 Cleaning up project directory and file based variables 00:01 ERROR: Job failed: command terminated with exit code 1
I tried reverting the changes in composer.json and then adding back the changes and updated the lock file but the error still shows up. I also tried changing the branch in which I was merging thinking maybe i was working in the wrong branch but that didnt work as well.
- 🇮🇳India divyansh.gupta Jaipur
@pfrenssen I have made changes in the classes requested and applied dependency injection and removed direct drupal calls and placed construct and create functions where ever needed and updated existing in some of them.
Please review this MR and let me know if there are any changes to be made from my side. - @divyanshgupta opened merge request.
- @debrup opened merge request.