@dastan56 Can you specify the exact composer command you tried to execute ? I cannot reproduce this myself.
Discussing the issue in Slack #drupalpod channel, @rfay has suggested that we might try to mount the necessary files inside the web container.
Also @shaal has confirmed he can replicate this issue.
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 → made their first commit to this issue’s fork.
guiu.rocafort.ferrer → created an issue.
guiu.rocafort.ferrer → made their first commit to this issue’s fork.
After updating the branch to be able to use the CI pipelines, it turns out the test is not quite right, and needs to be fixed.
guiu.rocafort.ferrer → changed the visibility of the branch 3429638-automated-drupal-11 to hidden.
Thank you @idebr !
guiu.rocafort.ferrer → made their first commit to this issue’s fork.
I found something interesting experimenting around:
I added a value to a configuration value of type weight ( in this example user.role.authenticated.weight ), bigger thant the PHP_INT_MAX value for 64 bit ( 922337203685477580722 ). The PHP_INT_MAX for 64 bit is 9223372036854775807.
Imported the configuration via drush cim ( it did not show any error ).
Export the configuration via drush cex.
Then i saw that the integer value that was exported, was set to the PHP_INT_MAX for 64 bits ( 9223372036854775807 ).
So, isn't the configuration validation suposed to throw an error when importing invalid configuration ?
Also, if this is confirmed, the imported value should not be modified after the import, so if the value is bigger that the PHP_INT_MAX value for that machine, the configuration should not be valid. In that case, i guess the validation should be implemented in a callback so the specific PHP_INT_MAX/MIN values for that php can be validated.
PD: I tried this in Drupal 11.0.5, i will try to replicate this using the drupal dev branch.
I have applied the changes in a new merge request to 7.x-1.x (3472254-wrongly-parsed-sentence-7.x).
@fmb I have checked that in the frontend the variables are not incorrectly hightlighted by the javascript code.
The issue fork have been merged in development. Marking as fixed.
I tried again in my local machine today ( yesterday i tried with a drupalpod instance ), and the redirecting issue seems to be a thing with the DrupalPod integrated browser, sorry about that.
The issue needs tests before it can be merged.
I have checked that the issue is still present in the current 8.x-1.x-dev branch, and that the patch fixes the redirection issue.
The problem i see is that, even though the loaded domain is the default one, the url is the same, i think that at least a redirect should be done to the domain that is actually being loaded, so i am setting this as needs work.
Tasks pending:
- Make an actual redirection so the url in the browser matches the displayed domain.
guiu.rocafort.ferrer → made their first commit to this issue’s fork.
@_tarik_ i don't think that storing empty third_party_settings around is a good idea. I am not 100% sure, but it might also introduce unnecesary dependencies in the domain not using the country_path functionality to the module. I think a better approach might be to alter de JSON:API response to include the empty values in the response, for example.
I am setting this issue as won't fix.
I am interested in co-mantaining the project, so i am changing the status to active again.
The local tasks menu now displays a "Languages" link in it. Setting it to needs review.
Hello @kartagis, thank you for the video, it was very helpful.
When you are adding the translation, now there are two different text areas. You are suposed to add the individual string translation in one, and the plural string translation in the second one.
I am attaching a screenshot to try clarify it. of couse i don't know turkish so i just copied the text in english to ilustrate the point.
Shouldn't the range constraints of PHP_MIN_INT and PHP_MAX_INT be defined in the integer type ?
We might then try to validate the configuration using the configuration value "system.site.weight_select_max", but i am not sure if this is possible... aside from having an adequate constraint plugin, it might require to add system.site to as a dependency to all of the configuration objects that uses type: weight
Thanks for looking at this.
This was fixed and available on localize.drupal.org for a while.
I tested it and added a translation for the string mentioned, and it works properly. I am setting the issue as fixed because of that.
We found out that the code was treating JSON:XXXXXX as a variable, this is a general problem where it is interpreting JSON:API as a variable name, when it is actually the drupal core submodule name.
I added an exception to the regular expression checking the variables, that would ignore any value starting with "JSON:" from being interpreted as a variable.
guiu.rocafort.ferrer → changed the visibility of the branch 3472254-wrongly-parsed-sentence to hidden.
The string id i wrote in the comment #7 was wrong, i did not realise the id in my local setup is different than the one in localize.drupal.org, so İ updated the comment accordingly.
Sorry about that.
I was not able to replicate this issue in a local environment:
I used main branch from drupal-infrastructure gitlab repository,
I used 3.0.x-dev branch for l10n_server module.
The string id i tried to translate, and had no problem is the #5052.
Since issue #3423189 have been fixed, granting my account the possibility of opting projects into security advisore coverage, i am moving the issue back to Drupal.org project ownership so it can be resolved.
Hi @avpaderno,
I removed the hook_form_alter. Please review.
Thank you for your patience.
guiu.rocafort.ferrer → created an issue.
Hi @avpaderno
I have removed the buildConfigurationForm and implemented a form_alter hook instead.
Marking as needs review.
I believe that the fix should be merged, so the breakString does not return a NULL operator, which causes a fatal php error, and then we can create a follow-up issue to discuss the acceptable characters in the provided values, as #59 suggests.
The merge request solves the issue implementing hook_module_implements_alter, and making sure the migration_plugins_alter hook from migration_plus executes before anything else. This way, when the core action module executes its migration_plugins_alter hook, the migration configuration will already include the merged shared_confguration from the group.
Setting the issue as needs review.
I have updated the test files since there were not the correct ones.
Hi @avpaderno
The thing is that this is made on purpose, and it is part of how this module work and makes sense. The fact that the configuration is in a separate settings form, allows the submodule from domain (domain_config) to override the settings for different domains. If i would add the configuration form at the block itself, then this module would not have any reason to exist, because the "Social Media Links Block and Field" module ( https://www.drupal.org/project/social_media_links → ) would have the same functionality.
If this is not acceptable to grant the security advisory coverage, let me know and we can close the issue, so we don't waste each other's time. I will try to qualify for the security advisory coverage when i have a chance to contribute another Drupal module and open a new issue.
So if i remove the buildConfigurationForm method for the class and implement a form_alter hook, would this be considered a better approach for this use case ? @avpaderno
The patch worked for me in Drupal 10.3.1
Hi @avpaderno,
I am a bit confused about what you are saying. SocialMediaBlock::buildConfigurationForm() is getting the form from the parent, and then adding a link element before returning it. The only reason i could think of this not being a good practice, might be the fact that the function is not adding any form element, it is just adding a link. I could remove the buildConfigurationForm method and add a form_alter in the .module file to add the link instead, would that be a more acceptable practice ?
In my case, i had a string value with the character "/" on it, so the regular expression was not picking it up and causing the operator to be NULL.
I think the main issue comes from HandlerBase::breakString, which should allways return an operator, even if none of the regular expressions apply. In that case, the value returns as an empty array, so i believe it should be safe to return any of the operators for that matter.
This issue does not only happen with numeric arguments, but also with string arguments that contain characters not present in the regular expression, so the fix should not focus on the numericArgument plugin.
Please review the new branch '3353786-breakstring-add-default-operator'. Setting the issue as needs review.
PD: Another different issue, which will require a different issue to address, is broadening the regular expression to allow for more special characters to be present, like the one that was causing the issue for me "/".
guiu.rocafort.ferrer → made their first commit to this issue’s fork.
Changing issue status to needs review.
Hi @avpaderno.
Thank you again for your feedback.
I have addressed the translatable elements issue as requested.
Please notice that i added a @codingStandardsIgnoreLine because phpcs did not seem to understand that the value of the variable is comming from a literal ( defined as a CONST in the class itself ), and complained about "Only string literals should be passed to t() where possible".
Regarding the block configuration, i am aware that this might not be the habitual way of handling it, but it is completely intentional, and part of how the module works. This module defines a block, that uses some settings defined outside the block itself. This is made on purpose to allow the domain_config subdomain to be able to define different variations of it for different domains. The link in the block settings form is merely a reminder for people that might be confused by how the module works, since there is another similar module "Social Media Block and Field" that works placing the configuration in the block itself. Please read the module description and let me know if you have any further questions about this issue.
Needs some more elaboration, and maybe some pictures.
Expand information on how to override the default css library.
Document how to write a custom preprocess function.
guiu.rocafort.ferrer → created an issue.
MR have been updated.
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.