search

prooph / event-store-http-client

PHP 7.2 Event Store HTTP Client Implementation
BSD 3-Clause "New" or "Revised" License
25 stars 5 forks source link

Schema inconsistency #4

Closed enumag closed 5 years ago

enumag commented 5 years ago

ConnectionSettings used by EventStoreHttpConnection has $useSslConnection parameter which is resolved to http or https here.

ProjectionsManagerFactory on the other hand is using EndpointExtensions constants.

I expect you'll want to unify this somehow.

prolic commented 5 years ago

Yep, on my todo-list, should be EndpointExtensions. Wanna change yourself and provide a PR?

enumag commented 5 years ago

Maybe we should use ConnectionSettings in ProjectionsManagerFactory? But I'm not sure - it seems that ProjectionsManagerFactory doesn't need the $requireMaster boolean.

prolic commented 5 years ago

Maybe we should use ConnectionSettings in ProjectionsManagerFactory? But I'm not sure - it seems that ProjectionsManagerFactory doesn't need the $requireMaster boolean.

Not sure about that.

enumag commented 5 years ago

Endpoint, Schema and UserCredentials are all needed in the EventStoreHttpConnection, UsersManager, ProjectionsManage and PersistentSubscriptionsManager so it would make sense to wrap these into one value object.

$requireMaster adds an 'ES-RequiresMaster' header to the HTTP requests created by EventStoreHttpConnection. Would this header make sense for the requests created by UsersManager, ProjectionsManage and PersistentSubscriptionsManager?

prolic commented 5 years ago

Nope, this header makes no sense there, but that's not the main issue I have. We could have an option that is unused in the managers, I think that's not a big deal. However for the QueryManager, you may need to configure your http client manually yourself to set much higher timeouts as the defaults.

enumag commented 5 years ago

Nope, this header makes no sense there, but that's not the main issue I have. We could have an option that is unused in the managers, I think that's not a big deal.

Then we can change ConnectionSettings to use EndpointExtensions and use it in all the managers, right?

However for the QueryManager, you may need to configure your http client manually yourself to set much higher timeouts as the defaults.

I'm not suggesting to add the HttpClient to ConnectionSettings. And there is nothing forcing you to configure everything the same way either so I don't see a problem here.

enumag commented 5 years ago

Hmm there are some more inconsistencies... Some classes (like ProjectionsClient) are using EndpointExtensions::formatStringToHttpUrl() while other classes such as AppendToStreamOperation are using simple string concatenation. I'm guessing this should be unified as well.

prolic commented 5 years ago

Hmm there are some more inconsistencies... Some classes (like ProjectionsClient) are using EndpointExtensions::formatStringToHttpUrl() while other classes such as AppendToStreamOperation are using simple string concatenation. I'm guessing this should be unified as well.

Yes, most of the code was written quick and dirty - I expect lots of inconsistencies currently. I need to review everything once tests are there.