Most things are there and working.

[Jaesin]

I have most things working in this branch.

[RoloDMonkey]

@Jaesin, I wish you had told me you were going ahead. I have been working on the same changes. I will give this a thorough review and provide feedback.

[RoloDMonkey]

We will need to update URL to the real repository.

[Jaesin]

The repositories section can be removed once an 8.x-1.x branch gets pushed to d.o.

[Jaesin]

Ah, my mistake. that is there until my pull request is accepted: dchesterton/marketo-rest-api#34

[RoloDMonkey]

@Jaesin, I understand why you are doing it but I am leery about making this module dependent on your Encryption module.

[RoloDMonkey]

@jyokum: Would you like to weigh in on this pull request?

[RoloDMonkey]

It looks like you are dropping webform support. This makes sense for now.

[Jaesin]

Yeah, the encryption module was made for this module specifically. The need is simple encryption so when api credentials get written to config they aren't publicly readable by a improperly configured server. It would be nice if there was a simple encryption mechanism in core and I wrote this one hoping to get it into a minor release.

Some people are using contact storage until something better comes along.

[Jaesin]

There are lots of tests but they rely on api credentials in environment variable. I have mine set up in PHPStorm and all tests are passing.

Required environment variables to get all tests passing.

marketo_ma_munchkin_account_id
marketo_ma_rest_client_id
marketo_ma_rest_client_secret
marketo_ma_munchkin_api_private_key

This can be set up using travis encryption but someone would need marketo sandbox credentials that they are comfortable with using.

[jyokum]

Starting to review

[mccrodp]

It looks like you are dropping webform support. This makes sense for now.

Would be great to integrate with EntityForm and/or YML form instead <- Nice roadmap goal :P Great job w/ this, won't have chance to review properly, but from what I saw before, looks very thorough 👍

[Jaesin]

I wanted to make adding additional integrations a trivial task but didn't have time to optimize for that.

It's a target of opportunity for sure.

[Jaesin]

This last commit fixes a config issue where if you uninstall mma_user, the config wouldn't uninstall. Then when you try to re-install, you get an error. It adds some refactoring on where the actual config names are stored.

[Jaesin]

Ah, because they are two different values.

const MMA_USER_CONFIG_NAME   = 'mma_user.settings';
const MARKETO_MA_CONFIG_NAME = 'marketo_ma.settings';

[RoloDMonkey]

Right. I guess I was reading too fast. :)

[jyokum]

I'm new to D8 and struggling a bit to get real testing going but here are a few thoughts from what I have observed so far.

encryption - Agree with @RoloDMonkey that the dependency to the other module is undesirable. If you have access to the server it's pretty easy to get at the real keys even when encrypted.
drush ev "var_dump(\Drupal::service('marketo_ma.api_client'))"

Would like to understand the rationale for renaming the submodules? marketo_ma_user -> mma_user

Prefer to leave default tracking method as Munchkin given it's the most straightforward setup and is all that's really needed if you're not going to also enable lead sync through one of the other submodules.

[Jaesin]

About renaming sub-modules. I didn't think the machine name was so important since the you can't upgrade a drupal 7 site but I'm pretty agnostic to their names.

About encryption, It's a common practice export config to files. The idea with encryption isn't to protect from someone that has server access, They can just log in and see the credentials on the configuration page if they have that. The idea is to protect against public access because of a poorly configured server. If a site doesn't protect sites/default/config properly, the yaml files could be publicly exposed.

We work with allot of enterprise clients but we don't have control over servers in many cases. So we have to do our part (due diligence) to protect sensitive data for our clients.

All that said, the encryption trait is only 33 lines of code, we could easily duplicate it inside the marketo_ma module until a sufficient encryption trait makes it's way into core.

Default tracking set to munchkin makes since.

[Jaesin]

I think most minor things can be sorted in followup issues but I added both RoloDMonkey and jyokum as collaborators to https://github.com/Jaesin/marketo_ma. Feel free to push any changes you guys need to make before accepting this pull request.

I still have about a week or so before I need to really implement for a client.

[RoloDMonkey]

If I understand correctly, it looks like we need a "go"/"no go" on two things before we can do a dev release and put this up on drupal.org:

1. Dependency on the encryption module.
2. Name of the sub-modules.

Which way are we going? I can do the work once I know what the decisions are.

[jyokum]

Let's keep the encryption module in and rename the submodules to follow pattern marketo_ma_XYZ

[RoloDMonkey]

Sounds good.

@Jaesin, Do you want to do the refactoring of the module names? I have some time today, if you need help.

[Jaesin]

@RoloDMonkey I am a bit busy today. If you have time it's great to get the help. Just have to make sure to rename mma_user.settings.yml back to marketo_ma_user.settings.yml.

[RoloDMonkey]

I will start on the refactor now.

[RoloDMonkey]

@Jaesin, refactoring is complete. You will definitely want to review and re-run the tests.

[Jaesin]

After one small fix everything looks good from the tests.

[RoloDMonkey]

@jyokum: I think this is ready to merge and become the new 8.x branch on drupal.org. Please let me know immediately if you have any questions.

[jyokum]

Great. I think I'd prefer to make this the 8.x-2.x release to correspond with 7.x-2.x being the move to REST API. Any objection?

[RoloDMonkey]

@jyokum: No objections: https://www.drupal.org/node/2610556#comment-11620597

:)

[Jaesin]

@RoloDMonkey Thanks for the help.