🇧🇷Brazil @murilohp

Just uploading an static patch of MR here and moving to NR

murilohp → created an issue.

damienmckenna → credited murilohp → .

Just added a CR for this issue, and hide the unnecessary branches. Moving back to NR

murilohp → changed the visibility of the branch 10.1.x to hidden.

murilohp → changed the visibility of the branch 10.2.x to hidden.

Just rebased the MR, moving back to NR

Just answered the MR comment, moving back to NR

Hey, I just created an MR with the patch from #2, I also added an unit test for the getSupportedEntityTypes, I know it seems out of the scope, but I think the unsupported entities can change in the future, so to add some coverage there I though it could be a good idea to cover it with tests, I also would like to add more coverage to the MetatagDefaultsForm class, but I think this would increase the solution here, and we can address this in a separate issue. I'm moving this back to NR to see what you guys think about it.

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

I was testing locally here, and I wasn't able to reproduce the issue, I found out the function was removed and according to the 📌 Remove Drupal\Tests\migrate\Kernel\MigrateSourceTestBase::providerSource() Fixed , the method was removed from the code, but still expected to be static, the way the method was declared is apparently right, so IDK how to fix it

Just added a new commit and right now it seems the tests all passed, I tried to keep the original idea from metatagFieldItem class by using the getValue and setValue, the setValue expects an array of key => value, so I changed it

Hey @damienmckenna, I was taking a look at the code in order to debug it and find what's the problem with the test, that's why I did some commits(sorry for the noise by the way, just trying to test it correctly).

Apparently the issue relies on the metatag/src/Plugin/Field/FieldType/MetatagFieldItem.php file, this commit added the change, the problem is actually to replace the $this->value by the $this->getValue(), the getValue function will load all the properties from the propertyDefinitions method(in this case it's just the value, and it returns an array with the following structure:

array:1 [▼
  "value" => "{"title":"[node:title] | [site:name]","description":"[node:summary]","canonical_url":"[node:url]"}"
]

If you take a look at the rest of the code, the code wasn't expecting an array where the key is the property and the value is the metatags. After I change the code, I was able to properly see the tests passing. But I saw some other tests failing, that's why I've done some rollbacks, IDK if all the tests were related to the changes I made, but I don't want to increase the work here, so I just rolled back to the way it was before I changed and I think this explanation might help you to move this forward.

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

Hey, just created a new test to validate the proposed solution, moving this to NR and removing the tag for now, thanks!

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

Some tests were failing yesterday, nothing related to the code, but right now it's all green

Does this also fix, 🐛 If notifications.email is a string, an error occurs Needs work ?

@quietone as far as I could test, this issue would fix this scenario, but it would require to re-save the form again somehow(maybe just a hook update), if a site has exported the configurations with the empty string, the site admin would have to update the configuration again and remove the empty string, and then the next time someone tries to save the form with an empty string, the error would not be happening anymore. I hope this answer your question.

I wasn't able to get in touch with the person who opened the first MR, so I've created a new branch and pointed to D11

murilohp → changed the visibility of the branch 3270892-if-you-dont to hidden.

Thank you @dww! I didn't know, this makes everything much easier, here's the results of the test-only job:

There was 1 failure:
1) Drupal\Tests\update\Functional\UpdateMiscTest::testEmptyEmailListNotification
Failed asserting that an array is empty.
/builds/issue/drupal-3440501/core/modules/update/tests/src/Functional/UpdateMiscTest.php:287
FAILURES!
Tests: 10, Assertions: 104, Failures: 1.
HTML output directory sites/simpletest/browser_output is not a writable directory.
PHPUnit 10.5.26 by Sebastian Bergmann and contributors.
Runtime:       PHP 8.3.9
Configuration: /builds/issue/drupal-3440501/core/phpunit.xml.dist
F                                                                   1 / 1 (100%)
Time: 00:03.023, Memory: 8.00 MB
There was 1 failure:
1) Drupal\Tests\update\Functional\UpdateSettingsFormTest::testUpdateSettingsForm
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
-Array &0 []
+Array &0 [
+    0 => '',
+]
/builds/issue/drupal-3440501/core/modules/update/tests/src/Functional/UpdateSettingsFormTest.php:102

For the testing part. I've added two tests.

1. The first one I added during the 🐛 Do not log an error message when no emails are specified for update notifications Closed: duplicate , I think it's important to cover how the field is saving the values right now(for more info pleas check this 🐛 Do not log an error message when no emails are specified for update notifications Closed: duplicate comment
2. The second test I did my best to actually replicate the same bug with all the conditions, it should be good enough right now, thanks to @quietone for pointing this up(for more context see this 🐛 Do not log an error message when no emails are specified for update notifications Closed: duplicate comment)

I wasn't able to change the first MR to point to 11.x so I just opened another one with right target. Do you guys know how can I open a branch without the fix to prove that the tests are failing? I've tried once but I wasn't able to dispatch a pipeline on the branch unless I open a MR. Moving back to NR.

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

Reviewed and commited the changes to the README.md, thank you all!

Thanks for the explanation @mortona2k and for the documentation! @paulrad, thanks for the patch, but as pointed by @mortana2k, you just need the package defined on your repositories section.

Since the all tests passed, I'm moving this back to NR.

FYI: I've created another branch with the test only, but I don't know how to trigger the build on gitlab for that branch only, and when I try to compare the branch, for some reason I can't open a new MR. It's been while since my last contribution and a lot has changed, if you guys have any docs or tutorials about how to properly use gitlab, it would be great for me. Thanks!

murilohp → changed the visibility of the branch 3460081 to active.

murilohp → changed the visibility of the branch 3460081 to hidden.

Hi! I was also having this issue and the MR fixes the problem, I was not able to write a properly test to trigger the exact error:
Error sending email (from mail@example.com to with reply-to not set).
This would require us to send an e-mail on a test suite, and this is not possible. So, instead of trying to replicate the same issue, the test tries to save the "Email addresses to notify when updates are available" field with an empty value and validates the exact thing that's being saved.

Before the fix, saving the field empty would result on something like:

$this->config('update.settings')->get('notification.emails') = [
  0 => ""
];

With the fix, it would be like:

$this->config('update.settings')->get('notification.emails') = [];

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

That's great, here's a static patch to be applied on composer

Just to keep things on track, here's a static patch with the fix.

murilohp → created an issue.

The #51 works great but it has some unnecessary files like .idea/php.xml, .idea/workspace.xml, .idea/vcs.xml, this new patch removes them making it easier to review.

I think the project is ready to be reviewed, moving to NR

murilohp → created an issue.

Added the gitlab ci template and fixed any remaining lint errors. Fixed

Just added a functional test to cover the settings form and the integration using drupal settings. Maybe in the future we could have another test.

Just uploading an static patch from the MR, after upgrading the core to 10.2, the hook update conflicted with an older one. If anyone else is having the same issue, I also had to update the hook update number in order to ensure the hooks will be executed.

FYI: drush ev "\Drupal::keyValue('system.schema')->set('system', 10100)";

I would just include some of that explanation in the similar modules section.

Good catch! Just updated the main page of the module adding what we discussed here.

Thank you!

Hey @w01f, first of all thank you for your interesting in this module, it's pretty new and it's awesome to have people testing and opening issues here.

Regarding your question, my idea was to create a simple module to just integrate adsense and Drupal through the script that adsense provides. This module does not have any blocks or further configurations besides the adsense id, the main adsense project that you asked, have a lot of features and provides blocks to configure where the ads would be placed. But since you can configure all the ads on the adsense admin panel, I thought it would be better for my projects to have just the simple integration and let my content editors to manage the ads on the adsense admin panel.

I think this will answer your questions, so I'll move this closed and if you have any further questions, please let me know!

murilohp → created an issue.

murilohp → created an issue.

Hey @divya.sejekan, thanks for your testing, but I don't know if this a problem here, you're gonna see this happening with almost all the icons.
For example: If my background is red, and I want to show the pinterest, the background color will conflict with the icon.

For this scenario you'll have to link another icon, and you can do it easily by accessing the admin/config/services/social-media and changing the default image. So I'll move this back to NR, and here is another patch with the latest stuff from the MR.

Just updated the MR by removing any twitter references on the configurations and on the form, since I did this, we would need a hook update, so I have commited one, the following patch is just the current state of the MR since it's hard to patch the MR directly(poiting to a commit)

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

The 📌 Drupal 10 compatibility Needs review landed so I'm closing this one.

murilohp → created an issue.

Hey, although #3 is working we're facing the same issue after saving and cleaning the cache. So I'm providing a new patch that changes the route and fix this issue.

hchonov → credited murilohp → .

Just uploading a static patch to be used on a D10.1.

Uploading a new patch that checks if the variant label already exists on the page.

Hey @clarkssquared thanks for the testing, but I think this error has nothing to do with this issue, I suggest you to open another issue to address this error, apparently there's a call to this non exist service on the post save of the file: Drupal\page_manager_search\Entity\PageManagerSearch. I'll move this back to NR to see what you think.

Here's a patch that makes the label field required, I don't know if it's the best solution but it's working properly right now

murilohp → created an issue.

Here's a patch with the proposed solution.

murilohp → created an issue.

Here's a new patch implementing an event subscriber, the old solution #4, wasn't working properly, now it should work correctly with the recent versions of drush on D10. This new solution is completely based on ✨ Ignore configuration storage collections? Needs work .

After a further investigation here, I found out an old custom module that creates a service that extends the schedulerManager, sorry for the noise here, the module is working as designed.

murilohp → created an issue.

Hey, I'm currently using #7 with version 2 of this module, since we're migrating the system to D10, I'm rerolling #7 to be compatible with D10.

Hey, just updating #4, there were some old jquery.once left on the js

Here's a patch implementing the proposed solution

murilohp → created an issue.

I had an specific scenario where the link_override__options did not exist on menu_link_content_field_revision table, so I've made this patch to skip the update when a field doesn't exist. I hope it helps.

Here's a patch with the proposed solution.

murilohp → created an issue.

Just uploading the MR as patch to properly fix a version and avoid any unexpected updates.

Uploading an static patch here since we cannot pin the MR and be able to use it.

Just uploading a patch here because we cannot pin the MR to be used in a project.

Uploading an static patch since we cannot pin a MR to be used.

Just uploading a static patch because we cannot pin the MR in a commit to use

Posting the static patch because using the MR doesn't allow pinning to a specific commit.

Just uploading a patch to keep things tracked in case the MR got updated

I was currently using the mr as a patch, but since it can be changed and affect sites on production without any announcements, it's better to use a patch.

Thanks @AndersTwo to open this issue, I'm also having issues with the drupal 10 upgrade, I'll update the description with the error that I found and I'll update the MR with the fix. I'm also moving this to NR.

Hey @mandclu, thanks for commiting this one, I was testing the module and I think there's one more fix left, according to the [#3180429], the field_widget_form_alter became deprecated and was removed on D10, here's a new patch with the correct hook.

Rerolling #3 to be compatible with the latest release.

Thanks @renatog, commited to 2.0.x.

Commited to 2.0.x, thanks

Thanks for your contribution @Kamlesh Kishor Jha! It looks good to me

We already have the 📌 Automated Drupal 10 compatibility fixes Fixed for D10, so I'm closing this one as duplicated.

I think this needs more information, I'm gonna postpone this one.

Hey @andypost I think you should go with the MR in this case. The MR have the test fixed(both for D9 and D10), and also incorporates the 📌 Deprecation warning 'Not marking service definitions as public is deprecated' Needs review (which can be closed as duplicated since the deprecation notice reported is for D10 and we can address this here), what do you think?

Just rerolling #45 to hal module 2.x

Commited to 2.0.x, thanks!

Thanks for your contribution! The patches and the MR looks good to me

This was commited on 7f8fcdd7, so closing as wont fix

Since we're moving to 2.0.x, this issue is not necessary anymore.

Commited to 2.0, thanks!

I was reviewing it here and it looks good!