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

[#65] Deprecate "createdBy" and "lastModifiedBy" properties #74

Conversation

arlina-espinoza
Copy link
Contributor

Closes #65 . Deprecate "createdBy" and "lastModifiedBy" properties, as they are not being used by the Drupal Edge module, and are not supported by Hybrid.
This PR adds deprecation comments and removes the properties from the test mock data as well.

@googlebot googlebot added the cla: yes Indicates CLA has been signed label Oct 25, 2019
Copy link
Contributor

@mxr576 mxr576 left a comment

Choose a reason for hiding this comment

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

Just because these properties are deprecated I would not remove them from the JSON files and the tests because non-hybrid orgs still support them and we should have test coverage for these.

@@ -38,6 +38,10 @@ trait CommonEntityPropertiesAwareTrait
/**
* Email address of the organization user who created the entity.
*
* @deprecated Not used in Hybrid orgs.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we used a similar, standardized format like Drupal 8 introduced which tells a deveoper which version XY was deprecated and when it will be removed? Also, if there is any replacement for the class/method/etc a developer can get information about it?

https://github.com/drupal/core/blob/b3273887e822d745ed277ce36cd07dc56e795d83/includes/file.inc#L49

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed it to:

@deprecated in 2.0.4, will be removed before 3.0.0. Unsupported in Hybrid.
@see https://github.com/apigee/apigee-client-php/issues/65

2.0.4 should be the next release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unsupported in Hybrid.

What does it mean exactly? What happens if I call these methods on an object returned from a hybrid org? Do they return an empty string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mxr576 The fields are empty so calling the methods would return NULL, as they are never initialized.

@arlina-espinoza
Copy link
Contributor Author

Just because these properties are deprecated I would not remove them from the JSON files and the tests because non-hybrid orgs still support them and we should have test coverage for these.

You are right, I've added them back.

@arlina-espinoza arlina-espinoza added this to the 2.0.4 milestone Oct 25, 2019
Copy link

@minnur minnur left a comment

Choose a reason for hiding this comment

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

This looks good to me. TravisCI is failing, looks like it is not related to this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indicates CLA has been signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants