Account created on 28 June 2020, almost 4 years ago
  • Software Engineer at CI&T 
#

Merge Requests

Recent comments

🇧🇷Brazil murilohp

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

🇧🇷Brazil murilohp

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

🇧🇷Brazil murilohp

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.

🇧🇷Brazil murilohp

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

🇧🇷Brazil murilohp

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

🇧🇷Brazil murilohp

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.

🇧🇷Brazil murilohp

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)";

🇧🇷Brazil murilohp

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!

🇧🇷Brazil murilohp

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!

🇧🇷Brazil murilohp

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.

🇧🇷Brazil murilohp

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)

🇧🇷Brazil murilohp

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

🇧🇷Brazil murilohp

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.

🇧🇷Brazil murilohp

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

🇧🇷Brazil murilohp

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.

🇧🇷Brazil murilohp

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

🇧🇷Brazil murilohp

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 .

🇧🇷Brazil murilohp

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.

🇧🇷Brazil murilohp

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.

🇧🇷Brazil murilohp

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

🇧🇷Brazil murilohp

Here's a patch implementing the proposed solution

🇧🇷Brazil murilohp

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.

🇧🇷Brazil murilohp

Here's a patch with the proposed solution.

🇧🇷Brazil murilohp

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

🇧🇷Brazil murilohp

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

🇧🇷Brazil murilohp

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

🇧🇷Brazil murilohp

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

🇧🇷Brazil murilohp

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

🇧🇷Brazil murilohp

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

🇧🇷Brazil murilohp

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

🇧🇷Brazil murilohp

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.

🇧🇷Brazil murilohp

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.

🇧🇷Brazil murilohp

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.

🇧🇷Brazil murilohp

Rerolling #3 to be compatible with the latest release.

🇧🇷Brazil murilohp

Thanks @renatog, commited to 2.0.x.

🇧🇷Brazil murilohp

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

🇧🇷Brazil murilohp

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

🇧🇷Brazil murilohp

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

🇧🇷Brazil murilohp

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?

🇧🇷Brazil murilohp

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

🇧🇷Brazil murilohp

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

🇧🇷Brazil murilohp

The module was refactored on 2.0.x, moving this to closed.

🇧🇷Brazil murilohp

Just closing this as fixed, commited on 2.0.x, thanks

🇧🇷Brazil murilohp

Just closing this one to clean the issues queue.

🇧🇷Brazil murilohp

Commited to 2.x, thanks

🇧🇷Brazil murilohp

Thanks for you contribution @ilya.no! It makes a lot of sense, I was reviewing here and looks good to me.

🇧🇷Brazil murilohp

This was fixed on the ^2 version, so closing this one.

🇧🇷Brazil murilohp

Commited to 2.x thanks!

🇧🇷Brazil murilohp

Thanks for the patch, I was reviewing here and it looks good, just changed the module_load_include('inc', 'onetrust_cookie_blocking', 'onetrust_cookie_blocking_js_list');, I'll commit this and release a new D10 tag soon, thansk

🇧🇷Brazil murilohp

For those who needs this on D10.0, here's a patch(basically what was commited on ef5c7f7)

🇧🇷Brazil murilohp

Thank you for your input here @apaderno!

For these applications, we need a project where, in at least a branch, most (if not all the commits) have been done from the user who creates the application. In this case, I can only see a commit where your username appears.

Oh I thought I would be able to apply after fixing the issues, my bad here.

Verify the email used by Git is the same email associated to your account on https://www.drupal.org/user/3650571/edit .

And thanks for pointing this, I haven't realized I was commiting using other e-mail
I think we can close this one as won't fix, since it doesn't make sense to apply for this role right now, I hope to get this in the future but not using this module, anyway, sorry for the noise here.

🇧🇷Brazil murilohp

This will be needed to be ported to version ^2, meanwhile, the following patch will apply on ^1 and will be compatible with drupal 10

🇧🇷Brazil murilohp

Thanks for your contribution @Jay Jangid, @arpitk and @urvashi_vora, I've reviewed the patch and looks good to me

🇧🇷Brazil murilohp

I'm gonna close this issue, this type of functionality will not work on simple_sitemap 4 and it doesn't make sense to be here.

🇧🇷Brazil murilohp

Adding a drush 11 compatibility and fixing some unit test errors

Production build 0.69.0 2024