New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TASK] Implement core admin api for solarium #625

Merged
merged 2 commits into from Sep 14, 2018

Conversation

Projects
None yet
4 participants
@timohund
Copy link
Contributor

timohund commented Aug 20, 2018

This pr:

  • Implements the CoreAdmin, which can be executed with several actions.

Example:

$coreAdminQuery = $this->client->createCoreAdmin();

$statusAction = $coreAdminQuery->createStatus();
$statusAction->setCore('techproducts');
$coreAdminQuery->setAction($statusAction);

$response = $this->client->coreAdmin($coreAdminQuery);

echo $response->getStatusResult()->getUptime();
  • Add's unit tests for the implemented CoreAdmin actions
  • Refactores the Request and Query to indicated if a query is coreIndependent. Which means that this query needs to be executed outside of a core context.

Resolves: #624

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 20, 2018

Coverage Status

Coverage decreased (-0.2%) to 90.952% when pulling c5544c3 on timohund:feature/master/624-support-core-admin-api into 54cad6a on solariumphp:master.

@timohund timohund force-pushed the timohund:feature/master/624-support-core-admin-api branch 7 times, most recently from 4cce322 to e12069e Aug 21, 2018

@timohund

This comment has been minimized.

Copy link
Contributor

timohund commented Aug 21, 2018

Hi,

@mkalkbrenner @thePanz @basdenooijer i've pushed a first version of the core admin api.

By now it is backwards compatible and i tried to implement it in a way that it fit's into the current structure of solarium. It would be nice to get your Feedback.

There are something where it would be good that we discuss them:

  • By now all Querys are in the QueryType package. Do you think it makes sence to split them into Admin in NonAdmin packages to have them logically separated? As it is implemented now, it is not seperated because i think this requires some breaking changes

  • The implementation of the core admin feature requires to distinguish between coreindependent and corerelated query types. By now every query was corerelated, which means that this Query allways needs to be executed in the context of a core/collection. This is now solved by passing a boolean value to the getBaseUri method. This is backwards compatible but i think it would be better to have two methods getBaseUri and getCoreBaseUri. This would require to make a breaking change in getBaseUri().

What do you think? By now everything is implemented to keep backwards compatibility but it might makes sence to introduce so breaking changes in version 5.

And for sure some examples and documentation is missing but i would add after we agree on a way how the implementation should be done.

*
* @return string
*/
abstract public function getType();

This comment has been minimized.

@thePanz

thePanz Aug 21, 2018

Member

you added the typehint for the other methods. the getType(): string; is missing here

This comment has been minimized.

@timohund

timohund Aug 21, 2018

Contributor

thx, fixed that

*
* @return bool
*/
public function getIsCoreIndependent();

This comment has been minimized.

@thePanz

thePanz Aug 21, 2018

Member

Missing return typehint

This comment has been minimized.

@timohund

timohund Aug 21, 2018

Contributor

thx, fixed that

@timohund timohund force-pushed the timohund:feature/master/624-support-core-admin-api branch from e12069e to 9577516 Aug 21, 2018

@mkalkbrenner

This comment has been minimized.

Copy link
Member

mkalkbrenner commented Aug 23, 2018

First of all, great work so far!

I'm interested in getting this in very quickly.

By now all Querys are in the QueryType package. Do you think it makes sence to split them into Admin in NonAdmin packages to have them logically separated? As it is implemented now, it is not seperated because i think this requires some breaking changes

In which way will this cause breaking changes?

The implementation of the core admin feature requires to distinguish between coreindependent and corerelated query types. By now every query was corerelated, which means that this Query allways needs to be executed in the context of a core/collection. This is now solved by passing a boolean value to the getBaseUri method. This is backwards compatible but i think it would be better to have two methods getBaseUri and getCoreBaseUri. This would require to make a breaking change in getBaseUri().

Even the name getCoreBaseUri() is wrong in the context of Solr Cloud collections.
I suggest to keep getBaseUri() for the core/collection (transparently, depending if it is a Solr Cloud or not). In addition we should introduce getServerUri() and remove the isCoreIndependent stuff.
(The Drupal modules evolved the same way.)

For sure, the documentation needs to be enhanced for getBaseUri().

@timohund

This comment has been minimized.

Copy link
Contributor

timohund commented Aug 23, 2018

@mkalkbrenner thanks for your feedback:

In which way will this cause breaking changes?

Because we would have two new packages below QueryType e.g. Server (for the server related querys like the admin query) and Core (for the core specific query, everything else then the CoreAdmin queries by now). I am not happy with these names by now but i would say we would need one package for server specific queries and one for the core/collection specific query. Or we keep the queries as they are and just add a new package for the Server specific.

Do you have a proposal there?

Even the name getCoreBaseUri() is wrong in the context of Solr Cloud collections.
I suggest to keep getBaseUri() for the core/collection (transparently, depending if it is a Solr
Cloud or not). In addition we should introduce getServerUri() and remove the isCoreIndependent
stuff. (The Drupal modules evolved the same way.)

Having getCoreBaseUri and getServerUri is good. With removing the "isCoreIndependet" stuff, do you mean just to remove the boolean value here? Because on the query/request level we need to have this information to be able to decide if "getCoreBaseUri" or "getServerUri" needs to be called.

As i understand you right i would:

  • Remove the option "coreindependent" from the query.
  • Move the CoreAdmin package into an own subpackage "QueryType\Server\CoreAdmin"
  • Have a property in the request to distinguish between server and non-server requests
  • In the Adapters call "getCoreBaseUri" or "getServerUri" depending if we have a server or core/collection specific request.

Is that right? Do you have other proposals?

@mkalkbrenner

This comment has been minimized.

Copy link
Member

mkalkbrenner commented Aug 24, 2018

Or we keep the queries as they are and just add a new package for the Server specific.

That's the backward compatible way I would prefer.

getCoreBaseUri and getServerUri will be new functions.
getBaseUri will be declared deprecated and directly call getCoreBaseUri for some releases.

@timohund timohund force-pushed the timohund:feature/master/624-support-core-admin-api branch 4 times, most recently from ad9ae57 to 3e6b4a4 Aug 24, 2018

@timohund

This comment has been minimized.

Copy link
Contributor

timohund commented Aug 24, 2018

@mkalkbrenner just pushed the current state, in the next step i'll add some more examples and complete the tests for some parts. If you have anything else please let me know.

@timohund timohund force-pushed the timohund:feature/master/624-support-core-admin-api branch 3 times, most recently from f2f5548 to 28d7f4a Aug 27, 2018

Timo Schmidt
[TASK] Implement core admin api for solarium
This pr:

* Introduces a new package "QueryType\Server" where all server related (not core specific queries) can be implemented.
* Implements the CoreAdmin, which can be executed with several actions.

Example:

```
$coreAdminQuery = $this->client->createCoreAdmin();

$statusAction = $coreAdminQuery->createStatus();
$statusAction->setCore('techproducts');
$coreAdminQuery->setAction($statusAction);

$response = $this->client->coreAdmin($coreAdminQuery);

echo $response->getStatusResult()->getUptime();
```

* Add's unit tests for the implemented CoreAdmin actions.
* Add's a method setIsServerRequest to indicate if a request was build from a server related query.
* Uses getCoreBaseUri for core/collection specific requests and getServerUri for server specific requests.

Deprecations:

* Endpoint::getBaseUri is deprecated now please use getCoreBaseUri or getServerUri now.

Resolves: #624

@timohund timohund force-pushed the timohund:feature/master/624-support-core-admin-api branch from 28d7f4a to ff93cdc Aug 27, 2018

@timohund

This comment has been minimized.

Copy link
Contributor

timohund commented Aug 27, 2018

@mkalkbrenner @thePanz i've added the feature, documentation and tests can you maybe review it and tell me if there is something left to merge it?

@mkalkbrenner mkalkbrenner merged commit e8cf9e4 into solariumphp:master Sep 14, 2018

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment