+1 from me as well. Setting RTBC
#12 says that the issue is solved with the MR. Setting RTBC because of this.
I've faced the issue as well. It seems like the 'Devel' maintainers have changed a lot of code in the DevelGenerateBase, while the WebformSubmissionDevelGenerate plugin extends it. Look on the commit.
Webform 6.2.2
Devel 5.2.1
It would be helpful to add a message in the release notes about this change. I had to discover on my own why a significant part of my site was down after a simple update :)
catch → credited taraskorpach → .
Looks mergeable now.
Somewhat off-topic: PHPCS suggests that the code $this->maxRows = $max_rows === NULL ? static::DEFAULT_MAX_ROWS : $max_rows; could be simplified to $this->maxRows = $max_rows ?? static::DEFAULT_MAX_ROWS;. Is it worth creating an issue for this?
Sounds great, thanks
I've noticed this issue as well. Since I'm relatively new, could you explain how such conflicts are handled within the Drupal issue workflow? Will the conflicts be resolved in the issue that is merged first, or is there another way?
Should there definitely be a dedicated configuration form? I mean, it would be odd if a contrib module required many forms for initial setup. Could we integrate it into the module's main form instead?
I've created another merge request to avoid overwriting anyone's changes. @acbramley, could you take a look? Is it finally a "property promotion"?
taraskorpach → made their first commit to this issue’s fork.
I've completed adding the logic.
However, I couldn't thoroughly test the functionality because I don't have a domain. I would appreciate it if you could explain how to test this effectively without a domain.
Btw, if the API returns any of the verdict keys, we will handle it properly.
The comments have been resolved. Please review one more time.
Added the ability for users to choose which Google account will be used. Nothing complex here therefore should be easy to review.
I've implemented the logic from the description to a minimum. Maybe someone can take a look?
I didn't add any complex logic, as we'll cover it in the child issues.
Marked as postponed because we need to finish at least 📌 Add a checking: do we have configured Google Service Account Active before we can continue.
This minimal implementation looks good to me. Marking RTBC.
Please update MR, it's one commit behind the main branch
The tests are running, but there are some warnings from phpcs and phpstan. We need to create new issues to fix them.
Marking this one as needing review.
Looks good to me. Marking RTBC.
As far as I know, d10 requires php8.1, shouldn't we add this requirement to the module's composer.json file? I think we could create a separate issue for that.
Class removed, tests passed -> no reason not to mark it as RBTC.
Seems done to me, so please review.
taraskorpach → created an issue.
Hope it would be changed right after a release.
Perhaps we need to change '^9' to to '^9.3' as well
I've removed the D8 requirement from the .info file. This will not allow you to upgrade to version 4.0.2 on d8 when the release will be made.
Ready for review.
Since this is a common template for contrib modules, I don't see any problem in marking the issue as RTBC. Btw, it's a really helpful thing, thanks!
taraskorpach → made their first commit to this issue’s fork.
Removing the PATCHES.txt file from the log
I'm returning the entire PHPCS log to the IS as @nikolay-shapovalov has mentioned above.
Btw, the .module file has been removed.
Confirm that the patch from #176 is installed correctly. Thanks!
I'm not entirely confident in what I'm saying, but how can we use the service locator pattern if we don't know which services to include in the container in the register method of ServiceLocatorTagPass?
In the get method, we use a service name obtained from the Settings. Does this mean we should include the entire container, or am I misunderstanding something? Please clarify this for me. Thank you in advance.
Yes, I've seen it. Issues arose due to the missing ContainerAwareInterface and ContainerAwareTrait, along with the new paths from 📌 Fork Symfony's ContainerAwareTrait and ContainerAwareInterface into core Needs work .
I haven't discovered what @arunkumark has done, but what would be the best solution here? Should we wait until the parent issue is merged, or should we just add the changes from this MR to the parent MR?
Based on #5 I've removed PHPStan messages and added the mentions of QueueFactory to the main CR → . So, marking it as needs review now.
taraskorpach → made their first commit to this issue’s fork.
Yep, I'll do this soon
I've added a minimal test to cover the issue's functionality. I'm unsure if it's worth splitting the code into a separate function, but that approach seems clearer to me. Marking it as 'needs review' so.
taraskorpach → made their first commit to this issue’s fork.
All threads seem to have been resolved. @nikolay-shapovalov, could you take a look at this?
I've discovered that the revisionOverview function in the NodeController doesn't provide an opportunity to display revision messages in the list, due to the following condition:
if ($revision->hasTranslation($langcode) && $revision->getTranslation($langcode)->isRevisionTranslationAffected()) {
This raises a question: is this logic necessary at all? I need another opinion to confirm my thoughts.
I'll update the MR according to your feedback. Thanks
I've updated the merge request, it's now ready to be merged. The tests have passed, but there is a stylelint warning. I'm not sure why it's showing up, as the log points to the 'padding' property, which seems correct to me.
taraskorpach → made their first commit to this issue’s fork.
1. The testAddDisplay method in the DisplayTest class tests the creation of a block display, whereas the method in the DisplayCrudTest class tests the page block. It doesn't seem logical to test different displays simultaneously; hence, we could retain only DisplayCrudTest since its class name more aptly suits the test in my opinion.
2. The DisplayCrudTest verifies whether the default block remains unchanged after adding a page block. Is it worthwhile to keep this test?
Regarding the merging with another method: all the methods utilize the straightforward randomView() function, which creates a view with a predefined display. In contrast, we need to add a display using the "Add {display}" button. I haven't found an effective way to merge these functions, so I hope you can.
The tests are fixed now, please review them. I've added a return type in accordance to the parent interface as well.
taraskorpach → made their first commit to this issue’s fork.
taraskorpach → made their first commit to this issue’s fork.
Hope it's okay within the Drupal community to interrupt in the middle of resolving an issue and make some MR changes. I just wanted to save you some time :)
I've updated the MR based on the comments. I've also added change records. Since this is my first time doing this, it would be better to have it reviewed.
taraskorpach → made their first commit to this issue’s fork.
Closing this as a duplicate according to #14.
Sorry for asking, but shouldn't I get a credit here?
I've also moved the UnpublishByKeywordComment. It would be good if you could take a look at it. By the way, the tests aren't running because an old plugin is missing.
taraskorpach → made their first commit to this issue’s fork.
I've updated MR according to your patch. Can I grant access to the MR, or does it solely belong to me?
I've moved changes from #2 into a pull request, along with some enhancements.
First, we need to verify whether the entity implements the RevisionLogInterface, and if so, use the common message instead of creating a unique one.
Currently, I'm uncertain about the necessity for test coverage, as I am not very familiar with Drupal tests. Nevertheless, I'm adding the 'tests' tag as a precaution. I hope someone can assist with this.
As I mentioned in #10, I'm unsure if the code is correct, so I followed the steps from the summary. Unfortunately, I'm unable to reproduce the issue.
My user has the 'Administer menus and menu links' permission but lacks the 'Access the taxonomy vocabulary overview page' permission. In this case, I have access to /admin/structure, and the 'Structure' link also appears in the menu.
I'm marking this as postponed since we need more information to reproduce it.
Should I create a new branch based on the 11.x?
I've created a PR to simplify Jelle_S's life. I'm not sure if the code will work properly, but based on #7, I'm marking the issue as 'Needs Review'.
taraskorpach → made their first commit to this issue’s fork.
It's TRUE by default, as mentioned here, isn't it?
Absolutely. Because of that, I've just added a sniffer rule to avoid unnecessary warnings. We just need a review rn.
I've added a README.md file. Please review it for grammatical accuracy and provide any necessary corrections.
taraskorpach → made their first commit to this issue’s fork.
Yeah, but these settings are part of a formatter field form.
As far as I know, we are unable to translate them because the issue 🐛 Entity view/form mode formatter/widget settings have no translation UI Needs review is still in progress.
I am modifying some arguments in the PHPCS command to utilize the appropriate phpcs.xml file during command execution.
Another method to bypass these errors is to use the ignore command, as in Drupal.Semantics.FunctionT.NotLiteralString.
However, creating a phpcs.xml file seems like a more suitable approach in this context.
I don't believe that removing translation functions is the best approach here. Someone may already have translated some strings on their sites, and removing these functions could probably cause some issues. This issue requires further consideration.
BTW, I'm currently running the PHPCS command in the project folder, and it isn't showing any issues. Could someone else try running the command?
I've changed the ID in the CountryConstraintFieldValidationRule.
To be honest, I didn't quite understand your point regarding the update path. Given that neither the constraint ID nor its name is recorded in the database or configurations, I'm unsure how we can update its value. Also, how does this affect users who are utilizing the constraint with the old ID?
Apologies for these basic questions; I haven't thoroughly explored the module yet.
taraskorpach → made their first commit to this issue’s fork.
I've fixed the module dependency warning and updated the MR.
Regarding the translation warnings I mentioned above, I've added a phpcs.xml file. This means you need to run the phpcs command directly in the module folder to allow phpcs to take into account all the provided exclusions.
I've created a MR.
The main objective has been achieved: The HTML now displays as follows: <time datetime="2024-08-26T12:00:00+00:00/2024-09-26T12:00:00+00:00">August 26 - September 26, 2024</time>.
I am uncertain about removing the 'date-display-range' class, as someone's styles might be linked to it.
Please review the changes. I have tested them with all available cases, and everything seems to work well. However, I am still not completely confident in the accuracy of the code.
taraskorpach → made their first commit to this issue’s fork.
I've created a MR.
FYI, I've added a phpcs.xml file to exclude warnings from the Drupal.Semantics.FunctionT.NotLiteralString rule.
If you have any suggestions for appropriately fixing these warnings, please do not hesitate to share them.
The issue needs review.
taraskorpach → created an issue.
The issue has been fixed in the following commit.
I've rebased the commits from the latest release.
The patch from #12 seems good to me; it has been applied, and the issue is resolved. Therefore, I'm marking this as RTBC.
I hope that the fix will be included in the next release, as it is quite critical.
taraskorpach → made their first commit to this issue’s fork.
It has happened to me as well in Drupal 10.1.5. The Drupal $project_data array does not contain 'title' key. As far as I understand it should be 'Drupal core'.
Should we ask maintainers to re-open the issue and change the priority too? I think that the problem is important when the error appears on the site randomly.
Btw, the updating to d10.1.6 has fixed the issue.
Just adding a re-roll of #33 for d10.1
LGTM. I applied the patch from issue #4 to enhance flexibility, as suggested by the author. Now, I can easily modify the token URL, region, and API URL to better suit my needs. Everything is functioning as expected.
+1 RTBC. I've encountered this problem with the region. Your solution is a good fit here
I've created a PR. Btw, I'm attaching a patch with PR changes for my own projects. I hope for someone's review.
taraskorpach → created an issue.
+
I've encountered an issue when trying to access the submissions page for a specific webform. I've update the patch to fix this problem. I don't believe this issue is related to problem from #10, so I haven't changed issue status.