🇺🇸United States @richgerdes

For context, the issue with the test is that the default form provides default values for the label, which will then get copied to the machine name field. Further updates to the label field don't processed by the machine name widget, which I assume is the result of the initial state of the form. This form is used by most of the test entities, so editing the form class isn't ideal. We could supply our own form class only for the machine name entities that doesn't have the default value generation. To me it seems that any entity that uses this form should be handling the default value generation in its tests though, and it should not be doing it in bulk, however yes that is likely to break tests elsewhere that depend on the random generation.

Ah yeah. Interesting. Are you using CKEditor 5? THat logic was written a long time ago around ckeditor 4, so I probably need to update the check for CKEditor 5. The above committes do take your suggestion and use the typeof call, which at least should fix the error, i'll probably need to review the structure of the CKEditor 5 functionality and get that working. The project this is on is still ckeditor 4, but its a good catch in advance of our upgrade.

Can you try the latest dev and verify that the error is gone? Probably still won't work with CKEditor 5.

@duckydan, that should fix it. Can you try the dev release and let me know if works?

Oh interesting. Looks like once wasn't replace properly when the module was upgraded.

This was released today in the 2.0 release → .

I've tagged and created the 8.x-2.0 release → .

@rajab natshah, thanks for putting this together.

I don't think we can mix 8.x-2.x and 2.1.x branches. We can just stick with the 2.x versioning. I think given there have been no functional changes and only changes to fix a small typo and test functionality, I think its safe to just declare a 2.0 release from the latest 8.x-2.x.

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!