🇺🇦Ukraine @taraskorpach

I suppose we cannot implement a factory completely based on the container.

We could add a method called setSource() to the WorkspacePublisher, in which we might set the needed Workspace to the publisher property.

In the WorkspaceOperationFactory, we should call the new service for WorkspacePublisher like "workspaces.publisher" and then call the method setSource() from itself.

Finally, the new method getPublisher() of WorkspaceOperationFactory should look like:

public function getPublisher(WorkspaceInterface $source) {
    return $this->publisher->setSource($source);
}

Please explain if we could use the DI factory pattern here or review my approach.

I have encountered a situation where the "body" field is not required and is empty in the original translation, but it is validated as mandatory at the reviewing stage.

#9 works helped me.

However, we need to add more tests to check the standard and required fields when submitting.

I've added a PR already.

taraskorpach → created an issue.

#39 has worked for me

I've managed to deprecate a config scheme and add a post_update hook. However, I couldn't find a test file where we should check for the deprecation message. Could you help me with that and approve the changes that have already been made?

taraskorpach → made their first commit to this issue’s fork.

Sorry for my ignorance :)

I've added a new PR on github.
Attaching a patch here as well.

Any credits for me? :(

So, we have the following line in the hook definition: @see \Drupal\Core\Language\LanguageManagerInterface::getFallbackCandidates().

Do we really need a deeper explanation of the parameter?

It's been 2 weeks with no response, so I think I can contribute. Needs to be reviewed now.

taraskorpach → made their first commit to this issue’s fork.

Could we use parameter documentation from LanguageManagerInterface::getFallbackCandidates?

I've added an MR, but here are some errors with unit tests.

Unfortunately, I don't know how to handle the Solr instance requirement during PHPUnit tests. I'll let you figure it out :)

taraskorpach → created an issue.

+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.