Changing issue priority to major, since i consider this to be an important enought issue.
guiu.rocafort.ferrer → created an issue.
guiu.rocafort.ferrer → created an issue.
The pipeline execution failed on may 7 because Linkit did not have a Drupal 11 ready version at that time.
Since may 24 linkit have published a Drupal 11 ready version, so executing the pipeline again should complete succesfully.
I have tried to find a way to execute the pipeline again for the merge request, but i could not find how.
I have applied the patch by @larowlan in the issue fork branch.
I have also added a new test to check the new functionality, in the process, i have modified slightly the link_attributes_test_alterinfo testing module to be able to provide different alters for different tests.
Marking the issue as Needs Review.
guiu.rocafort.ferrer → made their first commit to this issue’s fork.
Changing the issue status to needs review.
Fixed small typo in .eslint.json example
Thank you for the review @apaderno.
I have addressed the issues, and the code is ready to review again.
Changing the ticket status to needs work.
I've managed to fix all the phpcs errors except for one.
--------------------------------------------------------------------------------
167 | ERROR | All functions defined in a module file must be prefixed with the
| | module's name, found "coordinates_dd_to_dms" but expected
| | "smart_ip_coordinates_dd_to_dms"
| | (Drupal.NamingConventions.ValidFunctionName.InvalidPrefix)
--------------------------------------------------------------------------------
I am a bit concerned that changing that function name might break other contrib / custom modules that make use of it, so i am not sure about what might be the best approach for this.
I am thinking that when a new version including this change comes out, it should at least have a change notice, specifying that this function have changed name. Not sure if this is such big of a deal, but want to know what the mantainers position is in that matter.
guiu.rocafort.ferrer → created an issue.
Thank you for the feedback @heddn and @poker10
I have reverted the commits that added corrections for the style issues in the gitlab-ci pipeline, so those can be addressed in separate issues.
I did not disable the failing steps in the pipeline since there are marked as "allowed to fail", but if you believe they should be disabled until they are fixed in the separate issue, let me know.
IMO we could merge the issue with the fixes i already made, and then fix the remaining ones in different issues.
The eslint and phpstan validations are set as "allowed to fail", si i think it is ok to leave them on, but it is also ok to turn them off, and then turn them off in the specific issues we create as a follow up.
Hopefully the mantainers will give some feedback on the issue.
Merged changes
Moving the issue to Drupal.org project ownership issue queue, since there was no response from none of the project mantainers for more than 2 weeks ( see https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or... → ).
Note that i have an open issue for getting security advisory coverage for my account ( https://www.drupal.org/project/projectapplications/issues/3423189 → ). Hopefully when the two extra weeks given to the mantainers to answer pass, this one will be resolved.
Changing issue priority to Major as per issue priorities →
Changing issue Priority to Major as per issue priorities →
Changing issue status to needs review.
Changing the issue status to Crital as per issue priorities →
Changing issue priority to major per https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or... →
Thank you for the review @vishal.kadam.
The phpcs issues have been fixed in the 1.0.x branch.
For some reason the gitlab-ci template does not pick them up.
Ready for review again.
guiu.rocafort.ferrer → created an issue.
Hi @longwave, I understand your concerns about the performance issues, and the redundant http requests, i do believe too that this is not optimal, and there is room for improve.
This issue have been opened since 2015, and it has been difficult to push it forward and make it to merge with the development branch, so i am worried that addressing those issues would delay even more. I personally have a few sites that make use of the patch for a while now, and i believe some other people might be in the same situation.
So i believe it would be better to merge the issue into development, and then create a follow-up issue to improve the performance situation.
What do the other think about this ?
Thanks @abhishek_gupta1 !
Tested the patch file and works perfectly.
Setting the issue to Reviewed & tested by the community.
guiu.rocafort.ferrer → created an issue.
@smustgrave sorry about that, i will do that in future ocasions. Thanks for pointing that out, i didn't know.
Changing the issue status to Major as per issue priorities →
I am changing the issue priority as per Issue priorities → .
guiu.rocafort.ferrer → created an issue.
I have been looking a little bit more on the topic, and i found a configuration attribute system.site.weight_select_max. This is set to 100 by default on site install ( see core/modules/system/config/install/system.site.yml ).
One option to make this data type validatable, would be having a callback to check the value of system.site.weight_select_max, and sets Range constraints for min: -VALUE, max: VALUE.
This does not seem to be respected by core.extensions.yml, which sets a value of 1000 for the install profile used.
I see three possible ways to deal with this:
- Change system.site.weight_select_max to have a value of 1000, then the core.extensions would be valid
- Change the site install behaviour, to add the install profile with a weight value of 100
- Keep the core.extension weights as type integer, and leave it as an exception.
guiu.rocafort.ferrer → created an issue.
@Revathi.B, i had had to fix another issue i found in the module, so i just fixed this phpcs warnings while i was at it, so just ignore my last message.
I merged the issue branch into the development branch.
Closing the issue.
Thank you !
Hi @revathib, thank you for spotting out this missing detail in the module.
I see there are some phpcs validation warnings in the pipeline, which is very confusing, because the last commit was passing ok without any warnings, but the change you made, is only a change in a yaml file...
Would you mind fixing this 3 minor warnings, so we can merge the change cleanly in the development branch ?
I added the label field to the edit form, but i am taking in account the following cases, to avoid disrupting existing workflows and add extra steps when creating a tag container:
- If the label is left empty, the first tag ID will be used as the label, as before. I added a little description to clarify the behaviour
- If the label is specified, then this one will be used instead of the first tag ID.
The machine_name is still generated in the same way, and it cannot be changed.
I think this would be a great usability improvement for the module, because it allows to add friendly readable names to containers. If you have several containers, it can become complicated to know which is which.
IMO, it would be useful to be able to change the Label, but the machine_name should not be possible to be changed. This allows to add more meaningful names to the containers, rather than having to remember the container ids. This behaviour is widely used in Drupal Core, for instance, if you create a content type, then you can modify the label, but the machine_name stays the same.
There is already a label field in the entity type google_tag_container. So this would only require to add the textfield element in the edit form.
I will create a issue fork and give it a go, should be fairy easy to do.
I changed the TagContainerListBuilder class to extend DraggableListBuilder, and adapted the buildHeader and buildRow methods for the new class. For this to work, i had to add "weight" to the TagContainer entity annotation entity_keys.
I am not sure if the weight field in the edit form is really necessary, since it can be changed in the list already.
I am not complete sure, but i think we should write a hook_update, to clear the related caches for the entity type and force Drupal to pick up the changes from the annotation. I tried manually clearing caches, and seemed to work well. "config:google_tag_container_list" might be a good candidate for a cache tag to invalidate in that hook.
guiu.rocafort.ferrer → made their first commit to this issue’s fork.
I realized the functional test for testing the multiple width, height, both, none cases was wrong.
Fixed that in last commit.
Hi @larowlan, thank you for your suggestions.
I changed the formatter summary text, so there is one line for each width / height attribute that has been set.
I also joined the tests for the different cases, only width, only height, both, and none, in the same test method, changing the field config and loading the page again in the same method.
Waiting to see if all the pipelines run smoothly.
I added the CI configuration templates from DF. I also fixed most of the phpcs and phpstan most evident erros, but did not fix them all, because some of them might require some discussion in a different issue.
I believe this should be merged to the development branch, and then fix the remaining validation errors with different issues, so i am setting the issue to "needs review".
Setting the issue as needs review, so we can merge the branch and solve the issue.
I applied patch #3, and found the fatal error is caused by an infinite loop.
The setUrl calls getActiveDomain(), which loads the domain entity for the active domain. There was a domain_entity_load hook in the main domain, which calls the setUrl method for the loaded domain, thus creating the loop.
The getUrl() and getPath() methods already check if the value is set, and if not, they call setUrl and setPath, so i am quite sure removing this load hook in the main domain module is safe to do. I will create a follow up issue there.
Until this gets solved in the domain module, i added a hacky solution to stop the infinite loop from happening. In the setUrl i am checking if the function is being called from the domain_entity_load method, and in that case, it does nothing and just returns.
guiu.rocafort.ferrer → created an issue.
guiu.rocafort.ferrer → made their first commit to this issue’s fork.
guiu.rocafort.ferrer → created an issue.
guiu.rocafort.ferrer → created an issue.
The way i see it, external events, such as the install of a different module, are already triggering config changes in the ultimate_cron module, so i don't see why that would be different in the case of cache rebuilds. Without this functionality, a module that wants to start implementing a hook_cron would need to do one of the following:
- Implement a hook_update that triggers the discoverability of the new hook. I could not find any documentation at all about how to do this. The hook_update should be dependant on the fact that ultimate_cron is installed in the site too, if they don't list it explicitly as a dependency.
- ask the user to press the "discover jobs" button. not ideal in my opinion.
If we dedide on not implementing this, i believe that at least this should be documented, so module developers can implement a hook update to make ultimate_cron pick up the new hook, and create the corresponding configuration object.
The pipeline error seems to be related to this other issue ( https://www.drupal.org/project/drupal/issues/3401988 🐛 Spell-checking job fails with "Argument list too long" when too many files are changed Active ) and not with the merge request itself. The branch itself is mergeable, though.
Should we still merge the code back to the project ? Or should we make the pipeline run correctly first ?
IMO, this is a really small change, and it also have associated tests, so it should be safe to just accept the merge request.
The second option might require to merge the changes back to the issue branch, so the pipeline is updated with the fix from the issue, and then, push so it runs without hitting the error ? I am not completely sure about this.
Changing the issue status to needs review.
I added an additional functional test, checking that the width and height attributes are missing when the formatter settings have a null value in the width and height attributes.
I am removing the Needs tests tag, and changing the status to needs review.
guiu.rocafort.ferrer → made their first commit to this issue’s fork.
Hi @nadavoid, the merge request should now be good to go.
For some reason de phpunit functional tests started to fail with the error "Test was run in child process and ended unexpectedly".
I think that if we manage to fix this problem, we could merge the gitlab CI pipelines and fix the phpstan and eslint warnings in a different issue.
So, one of the errors is because they changed the save button label in the create content type form, now it just says "Save" instead of "Save content type".
The other 3 errors seem related to this issue https://www.drupal.org/project/drupal/issues/3316274 📌 Stabilize FunctionalJavascript testing AJAX: add ::assertExpectedAjaxRequest() Fixed
Since none of the errors are related with the issue, would it be safe to merge them, and tackle those in different issues ?
I added a new service domain_config.collection ( to be coherent with the module name and the rest of the services ), and added the function there. To avoid repeating code, i made the getDomainConfigName method static, so it can be used from the new service without duplicating the code. I also added a Functional test for the service method.
For some reason i cannot create a merge request for the branch i created, not sure what i should do about that...
I checked the issue branch and the commit for adding the Gitlab CI was present already. The branch was forked from the 2.0.x branch, so i just created a new merge agains the 2.0.x branch and the CI seems to be running now.
The thing now is that 4 phpunit tests are failing, but i cannot seem to reproduce them in my local environment.
Thanks for the clarification,
I failed to see that this service is not the right place to add this functionality due to the tight coupling with the DomainNegotiator.
Because the use of config collections issue is going to take some time, i agree with adding a new service "domain.config.collection" for this. Later, this service can be reused to perform some operations with the config collections once that is in place.
I will find some time to do this at some point today.
Thank you for having a look at it @samir_shukla
After having a look at this issue ( https://www.drupal.org/project/drupal/issues/2991337 📌 Document the recommended ways to obtain the database connection object Needs work ), i believe that the preferred way of accessing the database connection is:
$this->container->get('database');
Also see: https://www.drupal.org/docs/drupal-apis/database-api/instantiating-a-dat... →
I created a merge branch, and added the change there, so it can be merged to the main dev branch easily.
guiu.rocafort.ferrer → made their first commit to this issue’s fork.
I tried this to reproduce it using the steps in the issue description, with drupal:11.x-dev version, and the problem seems to be solved now.
Managed to write a test for it sooner than i expected. Setting the issue to needs review.
I am changing the issue status tu needs work, since i added the provided patch to the merge branch and tested it manually, but there is still need to write some tests for that functionality, which i will try to do soon.
I think i didn't set my point properly, so i will try again:
Generic issue: I have a specific configuration X. I want to know all the domains that override that specific configuration.
Particular use case: In this specific example i am exposing, i have a configuration that stores some credentials to an external rest api, which i use to import the contents via a migration. Each domain has a different endpoint url, and different credentials. so, i use a deriver to automatically generate a different migration for each domain that has this configuration overriden.
Potential use cases: I can think of some use cases for this, for example cron tasks that perform different actions according to each defined domain overriden configuration.
What i am doing now: I am loading all the domains, and iterating over them, then i call getDomainConfigName for each one, and then check if that configuration name is present or not.
I hope this time i was able to define more clearly the scope of the issue.
"I wonder if we could just allow prefixes to ignore the domain id portion of the config". I don't think this is possible.
As i see it, the fact that the domain and the language come before the configuration name makes it a bit difficult to retrieve a particular configuration name for all domains. If the configuration names had the format domain.config.{name}.{domain}.{langcode}, then it could be possible to load all overrides for a configuration using the prefix domain.config.{name}, but i guess this might be a big change in the domain_config module...
In this commit ( https://git.drupalcode.org/issue/domain-3409936/-/commit/c82f391bea0e657... ) i added a method that retrieves all domains, and checks the overrides for a particular configuration in all of them. I expanded the method functionality to include language overrides in this one ( https://git.drupalcode.org/issue/domain-3409936/-/commit/341c4227254c684... ).
I don't see how this could be done without loading and iterating over the domains, and in each one, iterating through the languages.
guiu.rocafort.ferrer → made their first commit to this issue’s fork.
Change the status to needs review.
I am changing the status to "Needs review", since i wrote tests for all the pager types, and also updated the documentation to reflect the new functionality.
Documentation: https://www.drupal.org/docs/8/api/migrate-api/migrate-source-plugins/mig... →
Add missing documentation on the paginator pager type.
The last commit adds test cases for the pager types urls, cursor, and page. The tests for paginator are still missing thought.
I also expanded a little bit the documentation on the pager types here: https://www.drupal.org/docs/8/api/migrate-api/migrate-source-plugins/mig... →
Documented some of the pager available types.
I updated the issue fork with the latest 6.0.x branch, so it is mergeable now. I also made a few changes so the Gitlab CI is available and runs de tests, and also fixed some of the tests.
Some of the changes i made, which might be arguable, are:
- When the item_selector does not exist, the getSourceData returns now an empty array instead of null. This way, the plugin acts as there is no rows to be imported and does nothing.
- When the item_selector is NULL or it is not defined, it is set to '' by default. In that case i believe the getSourceData method should return the whole sourceData contents.
- One of the tests failed because the $item_selector parameter for getSourceData was set to type string, but it was passing an integer value. Inside the function, it checked if the parameter is_numeric, to apply the backwards compatibility for depth selection. I changed the method definition to accept also integer values.
My plan is to next write some tests for the pager functionality, and go from there to fix potential issues and make sure it works ok, before merging into the main branch.
guiu.rocafort.ferrer → made their first commit to this issue’s fork.
guiu.rocafort.ferrer → created an issue.
John Cook → credited guiu.rocafort.ferrer → .
John Cook → credited guiu.rocafort.ferrer → .
John Cook → credited guiu.rocafort.ferrer → .
John Cook → credited guiu.rocafort.ferrer → .
John Cook → credited guiu.rocafort.ferrer → .
John Cook → credited guiu.rocafort.ferrer → .
John Cook → credited guiu.rocafort.ferrer → .
John Cook → credited guiu.rocafort.ferrer → .
Looks like the eslint job is failing because the current .eslint.json file present in the module code depends on the eslint-config-drupal npm package ( https://www.npmjs.com/package/eslint-config-drupal ). This package does not come in core package.json which is installed during the composer previous job, and it's artifacts are imported into the eslint job.
I am thinking a good approach would be just to make a .eslint.json which would reference the core existing .eslint.json file. This way we don't have to worry about updating the eslint configuration if it changes in core.
guiu.rocafort.ferrer → created an issue.
I updated the issue branch with the latest 2.0.x branch, and solved some merge conflicts.
I was not able to configure and run (yet) the Functional Javascript tests locally, so pushing to see if it passes the tests in the Gitlab pipelines.
I am planning on working on this in the following days.
guiu.rocafort.ferrer → made their first commit to this issue’s fork.
Closing the issue, because it was already fixed in 11.x version.
Changing the version to 11.x to fix it in that version first.
Fixed in the issue https://www.drupal.org/project/site_moderators/issues/3395571 →
Created tests for both of the modified forms, and run them successfully in DrupalPod.
guiu.rocafort.ferrer → made their first commit to this issue’s fork.