🇺🇸United States @richgerdes

Add details the hubspot module and difference between hubspot_api and hubspot.

@rondog469. That's great news, I'll leave it open a little longer but sounds like we can close it out in a week if there is no response.

This was already addressed in 🐛 Module breaks report Status Needs work .

Thanks for the report. This should be fixed now. It appears some of the Drupal 10 upgrade work wasn't committed from 📌 Automated Drupal 10 compatibility fixes Needs review .

Addressing this in 🐛 Module breaks report Status Needs work .

@mekal are you still experiencing this issue? If so can you try upgrading to the 3.0 release and let me know if that fixes it? If not, we can work on a fix.

@kyleadev or @ted-milker,

Are either of you still experiencing this? If so, can you try upgrading to the 3.0 version of the module and let me know if you are still having any issues?

@branram, are you having this same issue using the 3.x version of the module?

The 1.x release is no longer maintained. Closing this out.

I'm still open to this, if the hubspot api module is ready. At this point, I've implemented my own version of the client, in order to simplify the module, so transition should be easier. This should be done as part of 3.x or even probably new 4.x version since we will be adding a new dependency. Marking Postponed for now.

@christian.wiedemann, is this still an issue on 3.x? If so I can get it addressed, otherwise, let's close it out.

At this point the 3.0 version is the only maintained version and uses the updated Hubspot APIs. Closing this in favor of the 3.x branch.

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

For the redirect_url, this should match the application domain with the /hubspot/oauth path. Something like https://www.example.com/hubspot/oauth. Once the user authenticates in hubspot, they are sent back to this url so Drupal gets the OAuth credentials for making api calls. Please make sure you've configured your application redirect_url correctly within the application config on Hubspot.

If this doesn't work, please let me know.

Are any of you still running into issues with this? There should be no reason for the api to get disconnected. There are a few cases though, which may show in your logs

• Something along the lines of "Does not have the permissions..." or "Needs scope...", which requires updating the hubspot config and auth setting of the application in Hubspot to add the needed scopes
• A message about "Invalid Client Secret" or similar, which means the Drupal config changed or the hubspot app instance was disabled.
• A message about the oauth token being out dated or not having permissions, which indicates you need to reconnect your site to your hubspot organization.

There may be a few others. In the case of the "Hubspot api has not been configured" message that was initially reported, the hubspot token is a record of the app being connected to hubspot. It's stored in Drupal's state and shouldn't get removed unless someone clicks the "Disconnect Hubspot Account" button on the settings form. This token does expire, and could be getting dropped then. You can see the the expiration date of the token with drush state:get hubspot.hubspot_expires_in. If something goes wrong with the token refresh, it should trigger an error and not wipe out the expiration timestamp or access token.

If you see it happen again, It would be hl

@gbyte, There is an existing hook for this in webform. Please see hook_webform_handler_invoke_alter() and hook_webform_handler_invoke_METHOD_NAME_alter. In your case, you would be looking for the "post_save" method name. I don't know if we need to duplicate that function in this module. Here is an example using your code above.

hook_webform_handler_invoke_post_save_alter(WebformHandlerInterface $handler, $args) {
  $update = $args[1];
  if (empty($webform_submission->getData()['submit_to_hubspot'])) {
    $handler->disable();
  }
}

I do see value in offering a hook to alter the submission data from the Hubspot handler before its sent to the api. I think there is a case for altering the entire complied for data before submitting too. I've opened ✨ Add hooks for altering form data before upload to Hubspot Active for that.

richgerdes → created an issue.

+1 from me for this feature. I think this will be great. I've reworked the logic a little in order to handle this without adding additional services or adding extra parameters to the hubspot service call.

Since the webform handler already has the entity type manager, I'm inflating the file there and passing it in as the form value. I then changed the logic for the upload to look for the existence of the file instead of iterating over the inputs. This allows us to avoid looping over the elements and not complicate calls for non "form" based use cases. Additionally, I have included an added feature for down casting entities into strings if they are passed in as a form value, converting them into their labels. The latest PR also includes added logging in the event that the file upload does not succeed. I'm opting to skip the item, and continue with the form submission, as I'm not sure there is a good way to inform hubspot that the file upload failed.

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

This has been addressed in 🐛 ArgumentCountError: Too few arguments Active .

FYI, the does not use API keys, it does use the legacy "Public" application system, but that's not the same. The Drupal 8+ version of this module has never used API keys.

Reviewing the changes in this PR, the changes add support for the new "Private" application system, which uses OAuth2 which is a different scope. There are a lot of complex changes here, and I need to reconcile the impact this has on existing instances.

Additionally I need to sort out the patches what changes differ from the api implementation. There are a few requirements though, which don't appear to be covered by the changes that I see in the MR. First and foremost, we can't break production sites. It looks like the current code drops some state data and config options, likely breaking support for existing apps using the public app model, which is unacceptable. We need to continue to support the public app system while adding support for the legacy private apps and the new app eco system. However, neither of these are required based upon the email above, and HubSpot is committed to maintaining the existing legacy apps, but they might not get new features added to them.

If we do need to make a breaking change and drop support for the legacy app system, we will likely want to do that with a release of a new branch of this module, to make it clear to developers and site builders that there are breaking changes.

I ran into some issues with the ajax state of the api and how it was being processed. I've also fixed that as part of this PR.

Thanks for reporting the change. I've updated the default config for the site scopes.

@danflanagan8, thanks for the patch. This is a good improvement! Reviewed the schema, this looks like a good start.

@dan2k3k4 are you still having this issue? It seems like your webform handler was likely not fully configured with the correct hubspot form. If you are still having issues with this form can you share the webform config and an extended stack trace?

Patch from #4 is unhelpful, it doesn't handle the error or address the problem, just silently skips submitting the form. This isn't a good way to solve this problem. The admin needs to be informed that the submission didn't process.

I have tested this and recently reinstalled the module locally a few times. I have not been able to resolve this, but it does appear that it is a non issue. The suggestions here are all good, but summarizing them.

1. Check logs and verify that the module was installed correctly and no errors occurred during installation.
3. Verify the module is installed. Either via web ui at /admin/modules or cli with drush pml --status=Enabled --no-core
4. Try to clear cache so that the route map is updated. Either through the web ui at /admin/config/development/performance or cli with drush cr

Closing and linking to the other issue. Thanks for the review here.

@monymirza, thanks for the patch! This is a good fix. I like using the create this way.

Thanks for the patch!

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

richgerdes → created an issue.

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

I was able to get the test past the initial failure, however, since we now use an entity and not a session object, there are other dependencies, including the field definitions, which aren't loaded in the unit test scope. We could mock the full User Entity, but this seems overly complicated and costly. Alternatively, we could move the test to a kernel or functional test so we get access to the container and entity schema. Not sure what the core best practice recommendations are here though. I would think we would copy the required test scenarios to the new test and move it to the Kernal Space, if that's enough to bootstrap what we need.

Patch looks good.

Thanks for taking the first pass on this. Changes are pretty good.

I've added back support for Drupal 9 and 10, so we can continue to provide features to those versions. I've also switched to using dependency injection in the classes instead of calls to \Drupal.

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

Thanks for the bug fix and testing. Good catch on the missing file.

It doesn't look like there are any major issues with the code, as you noted, so I've marked the version compatible. I've dropped Drupal 8 support, since sites should consider upgrading.

@weseze, we can't just drop support for Drupal 9/10. Sites will still be using it. We need to keep that support, unless there is a breaking change.

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

Hi, yes, working on Drupal 11 for this. Sorry for the delay. Handling it over in 🌱 Drupal 10 -> 11 status? Active

@jantoine and @c7bamford,

Hope you guys are still using the adva module. Did some work on this and merged it in today. LMK if you get the chance to test and find any bugs.

Closing this and linking to the queue api issue.

@steveoriol thanks for the patch. @realityloop and @yuvania, thanks for testing!

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

@priti197 @alphex , thanks for the report and patch.

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

Hi @seth.e.shaw. I hope you found a solution to this, but my take is that we shouldn't disrupt the media access permissions as they stand. Advanced access should be used instead of the default. Basically don't assign the "view media" permission since this grants visibility to all "Media" but instead use adva to grant access to only the media you want the user to have. We shouldn't be trying to blacklist access or to work around what core is doing by default. Does that make sense?

Thanks for the corrections!

This is a duplicate of 🐛 Error when trying to save hubspot handle form Needs review , this change was already merged there.

@all,

Thanks for the hard work and Patch. Changes look good to me. Excellent find and fix.

@danflanagan8, can you provide a copy of your config and steps to reproduce this?

I'm not sure this is a duplicate.

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

Thanks for the fix!

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

I like the solution here, but as a better idea, I think moving this to a function would make things clearer. I've updated the PR to provide helper functions for creating the plugins and validating the configs. This should make it more straight forward, and prevent issues in the future.

richgerdes → created an issue.

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

@neclimdul, thanks for the patch! Changes look good. I have merged them

Adding issue 📌 Depricate Drupal 9 Support Fixed for removing Drupal 9 support.

I made two minor adjustments to the tests. The first was to prevent the default name from being set for string id entity type. This didn't break any other tests, which is a good start. The second change resolves a test validation. For some reason the text for the assert wasn't right. The expectation matched the output, but not the message text. I've updated the message in the validation so things are working now.

I reviewed the CR → and i believe it is sufficient.

Marking this as need review, so we can move it forward and get review. I don't believe there have been no major functionality changes since @quietone's review at #125.

Adding patch following reroll against latest core and recent changes

I did some work on this. I resolved the merge conflicts and the code quality failures on the PR. I also sorted out some of the test errors. The only major item remaining from the PR right now is the new test we are adding which fails. It appears that some alternative value is getting set, and not the on that we want.

I will work on getting simple test working local with xdebug on my new computer tomorrow and see if I can sort that out. But what i believe is happening is that the EntityTestForm is setting a name randomly. This is probably getting set in the id field as well. Our test tried to update the value of the name field and expects that to update the id. However, the value is not be updated as expected. I'm working under the assumption that the value that is resolved in js is the value of the original random name/id generated by the form.

IMO, the form shouldn't be handling setting the default value here, instead a function that submits the form with a random value would be better. Its implemented this way in the workspaces test and it seems much cleaner. However, this would likely require reworking a lot of the entity_test related tests, so I'm not sure we want to go that route. My plan is to go in with a debugger and see exactly what is being saved in the field and figure out how to bypass it being set or clear the value. Open to suggestions or feedback.

reviewing the summary in #125, I think the only major work besides the tests is to review and finalize the CR.

@norman.lol,

Regarding where the random values come from, I assume it is the logic in the EntityTestForm → , which is working as intended to generate a random string. This makes sense, but is a little tricky to bend to our will. The random value generation should probably have been supplied by the scenario where the form is submitted, but alas its done the way it is.

So how do we fix it?

richgerdes → created an issue.

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

I've processed the remaining rector items. The remaining items should be addressed later as they are BC breaks, and will cause a drop in support for Drupal 9 (Which is fine, but should probably be done in a new issue, since its otherwise not required for D11 support.)

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

Thanks!

I like this idea, the code looks good. I think the only thing I would add is to make the example hook in the api file include some logic as an example of how this can be used by a developer.

Hi, love the idea, and i struggled with this when I was first implementing the code here.

For contributed modules, we can't depend on the user using drush, so i can't rely on just the deploy hook to update the reference.

I personally have never used deploy hooks before, and given that this would prevent the default data from being generated for non drush users, I don't know if its the right approach. Also adding this as is would result in a redundant run of the value generation. Drush will still run the hook install during the deployment.

If you can find a set of changes which handles both drush and non drush users, i'd be open to it.

Hi @svendecabooter and @avpaderno,

I have been super busy recently and not actively using this module on any projects. I would love to have some assistance.

@svendecabooter, I've added you as a maintainer for now. I didn't see any work on issues for this project. Are you actively using the project for any sites or are there any things you want to get resolved?

I think the priority for this module is to upgrade the project to Drupal 11, and create a release, then focus on the items remaining on the 🌱 8.x-1.0 release plan Active .

Please reach out to me over slack if you want to bounce ideas off of me.

Thanks!

Oh interesting. Seems like there was a merge conflict or something when I tried to merge it. I've merged manually via cli and resolved the conflicts.

@codebymikey,

"Current maintainers" is just me. I have been busy, so I definitely would be open to some help.

It looks like you are active on a few of the recent tickets. I reviewed your work on 📌 Improve D9/D10 compatibility Active and 🐛 Use AccountProxyInterface rather than AccountProxy Needs review . The patches look good. I have no concerns with you helping out. I've merged the two issues along with an update to the ci job.

I've added you as a maintainer. I think the only major item left that we need to work on is 📌 Automated Drupal 11 compatibility fixes for redirect_node Needs review . If you have time, the automated patch may be a good thing for one of us to focus on. You can always reach me on Drupal Slack if you need anything.

Thanks for the hard work and patches.

The patch for the ci job is great. Will be nice to validate the code. I've addressed the additional items reported by the ci jobs here as well.