Skip to content
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

Fix script file issues and move script classes into namespace #1028

Merged
merged 5 commits into from
Jan 8, 2016

Conversation

mortenhauberg
Copy link

Elastica\Script and Elastica\ScriptFile extends Elastica\AbstractScript.
There are instanceof checks and type-hints for Elastica\Script in different classes.
I've changed the checks and type-hints to Elastica\AbstraceScript, and made Elastica\ScriptFile extend Elastica\Script.

@ruflin suggested to move the scripting classes into a namespace of its own.
We decided to keep the old classes, and have them extend the new classes in Elastica\Script for BC reasons.

Old New
Elastica\AbstractScript Elastica\Script\AbstractScript
Elastica\Script Elastica\Script\Script
Elastica\ScriptFile Elastica\Script\ScriptFile
Elastica\ScriptFields Elastica\Script\ScriptFields

Please let me know if anything needs changes :)

@ruflin
Copy link
Owner

ruflin commented Jan 7, 2016

@mortenhauberg Can you update the CHANGELOG.md?

* @author Nicolas Assing <nicolas.assing@gmail.com>
*
* @link https://www.elastic.co/guide/en/elasticsearch/reference/current/modules-scripting.html
* Kept for BC reasons.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a @deprecated message here? Check some other files which are deprecated for some examples.

@ruflin
Copy link
Owner

ruflin commented Jan 7, 2016

LGTM. It would be nice to have at least a deprecated message in the docs in all classes which are deprecated. Here is an example: https://github.com/ruflin/Elastica/blob/f124e598228a6b8a6c249fde6afc07ea7eb1a771/lib/Elastica/Query/Bool.php

@mortenhauberg
Copy link
Author

👍
Something like this?

ruflin added a commit that referenced this pull request Jan 8, 2016
Fix script file issues and move script classes into namespace
@ruflin ruflin merged commit 2b4eb2c into ruflin:master Jan 8, 2016
@ruflin
Copy link
Owner

ruflin commented Jan 8, 2016

@mortenhauberg Merged. Thanks. Perhaps in the future we can also trigger an error so it shows up in the logs:

trigger_error('Elastica\Query\Bool is deprecated. Use BoolQuery instead. From PHP7 bool is reserved word and this class will be removed in further Elastica releases', E_USER_DEPRECATED);

@mortenhauberg
Copy link
Author

Thank you. I'll create a PR for triggering errors next week :)

@ruflin
Copy link
Owner

ruflin commented Jan 8, 2016

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants