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

Replace deprecated Organization\Teams api calls #860

Merged
merged 8 commits into from
Apr 20, 2020
Merged

Replace deprecated Organization\Teams api calls #860

merged 8 commits into from
Apr 20, 2020

Conversation

lolos
Copy link
Contributor

@lolos lolos commented Apr 13, 2020

This Pull Request fixes the API calls for the functions:

  • \Github\Api\Organization\Teams::show
  • \Github\Api\Organization\Teams::update
  • \Github\Api\Organization\Teams::remove
  • \Github\Api\Organization\Teams::members
  • \Github\Api\Organization\Teams::check
  • \Github\Api\Organization\Teams::addMember
  • \Github\Api\Organization\Teams::removeMember

based on the official Github's v3 API documentation:

since those functions incorrectly threw Github\Exception\RuntimeException: Not Found

- \Github\Api\Organization\Teams::show
- \Github\Api\Organization\Teams::update
- \Github\Api\Organization\Teams::remove
- \Github\Api\Organization\Teams::members
- \Github\Api\Organization\Teams::check
- \Github\Api\Organization\Teams::addMember
- \Github\Api\Organization\Teams::removeMember
@lolos
Copy link
Contributor Author

lolos commented Apr 14, 2020

@acrobat @Nyholm
any suggestion for the backward compatibility failed check?

Should I make $organization optional or create a PR to v3-dev branch?

@staabm
Copy link
Contributor

staabm commented Apr 14, 2020

any suggestion for the backward compatibility failed check?

we could add the organization as a optional argument, and also adjust the implementation to be BC compatibel, e.g.

instead of

    /**
     * @link https://developer.github.com/v3/teams/#list-teams
     */
    public function show($team, $organization)
    {
        return $this->get('/orgs/'.rawurlencode($organization).'/teams/'.rawurlencode($team));
    }

use

    /**
     * @link https://developer.github.com/v3/teams/#list-teams
     */
    public function show($team, $organization = null)
    {
        if ($organization) {
            return $this->get('/orgs/'.rawurlencode($organization).'/teams/'.rawurlencode($team));
        }
        return $this->get('/teams/'.rawurlencode($team));
    }

.gitignore Outdated Show resolved Hide resolved
@GrahamCampbell
Copy link
Contributor

GrahamCampbell commented Apr 14, 2020

Don't worry about StyleCI. Its fixes will be automatically applied when the PR is merged (assuming the package author has enabled that option).

@lolos
Copy link
Contributor Author

lolos commented Apr 14, 2020

@staabm I've changed it to your suggested way but keep in mind that in a next version, the $organization should be set as required since the api calls don't work without an organization set.

Copy link
Collaborator

@acrobat acrobat left a comment

Choose a reason for hiding this comment

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

@lolos Thanks for the PR, it already looks good! Just one comment about the current setup.

lib/Github/Api/Organization/Teams.php Outdated Show resolved Hide resolved
@acrobat acrobat linked an issue Apr 15, 2020 that may be closed by this pull request
@lolos lolos requested a review from acrobat April 20, 2020 14:59
@acrobat acrobat changed the title Fix \Github\Api\Organization\Teams calls Replace deprecated Organization\Teams api calls Apr 20, 2020
@acrobat acrobat merged commit d3d6756 into KnpLabs:master Apr 20, 2020
@acrobat
Copy link
Collaborator

acrobat commented Apr 20, 2020

Thanks @lolos! And congrats on your first contribution! 🎉

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

Successfully merging this pull request may close these issues.

Support changes to teams API
4 participants