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

Service api changes #3000

Merged
merged 5 commits into from
Jun 18, 2018
Merged

Service api changes #3000

merged 5 commits into from
Jun 18, 2018

Conversation

sbossarte
Copy link
Contributor

@sbossarte sbossarte commented Jun 16, 2018

This is my PR incorporating some changes mentioned in #2992. A detailed outline of my changes are at the bottom of this comment.

Most all of the group-related functions in the Tags service have tagGroup in their name, opposed to the other services that have 'group', and I figured asking if deprecating all of those would be a good idea in order to rename them, or if leaving it alone would be better for anyone maintaining things. If a change is preferred, I wouldn't mind making that change and adding it to this PR, or a future one if need be. For now, my change to the Tags service has followed its own naming convention instead of the other services.

Also, in the initial issue, I pointed out the Structures service and its lack of a deleteStructure() function. Though it's worth mentioning that after looking through the source, I now understand why that service is built the way it is, and how my previous assumption doesn't apply in this instance.

Changes within this pull request were broken into three commits:

  • Updated variable name in Tags Controller to better reflect actual use.
    • /craft/controllers/TagsController
      • Modified actionDeleteTagGroup() to better name sectionId to groupId.
  • Added some service functions to handle deleting objects by a reference or by id.
    • /craft/services/Categories
      • Added deleteGroup(), which now contains the bulk of the logic that deleteGroupById() had.
      • Modified deleteGroupById() to forward to deleteGroup() after retrieving and verifying the group.
    • /craft/services/Tags
      • Added deleteTagGroup(), which now contains the bulk of the logic that deleteTagGroupById() had.
      • Modified deleteTagGroupById() to forward to deleteTagGroup() after retrieving and verifying the group.
    • /craft/services/UserGroups
      • Added deleteGroup(), which now contains the bulk of the logic that deleteGroupById() had.
      • Modified deleteGroupById() to forward to deleteGroup() after retrieving and verifying the group.
  • Updated AssetTransforms service to match the 'delete' and 'deleteById' convention.
    • /craft/services/AssetTransforms
      • Added deleteTransformById() which will accept an ID, and forward to deleteTransform() after retrieving and verifying the transform.
      • Modified deleteTransform() to accept an AssetTransform object.
      • Modified deleteTransform() to raise deprecation warnings when called with an integer ID.
    • /craft/controllers/AssetTransformsController
      • Modified actionDeleteTransform() to call the updated deleteTransformById() function of the AssetTransforms service.

@brandonkelly brandonkelly merged commit d734047 into craftcms:develop Jun 18, 2018
@brandonkelly
Copy link
Member

Thanks a lot!

I wouldn't mind making that change and adding it to this PR, or a future one if need be. For now, my change to the Tags service has followed its own naming convention instead of the other services.

We’d welcome the PR, but yeah it would be a bigger ordeal. There may be other classes that have similar inconsistancies as well.

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