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

Release another update? #57

Closed
philcarbone opened this issue Aug 12, 2016 · 27 comments
Closed

Release another update? #57

philcarbone opened this issue Aug 12, 2016 · 27 comments

Comments

@philcarbone
Copy link
Contributor

Hello,

Is it possible to release another composer update for this? Looks like its been a while and there are some good changes in the code :)

@frank-herbert
Copy link

Try add this repo in your composer.json to download the package directly from GitHub

    "repositories": [
        {
            "url": "https://github.com/jumbojett/OpenID-Connect-PHP.git",
            "type": "git"
        }
    ],
    "require": {
        [...]
        "jumbojett/openid-connect-php": "master-dev"
    },

@rasodu
Copy link
Collaborator

rasodu commented Aug 12, 2016

@jumbojett We need to decide version number that we will use for new release. Composer recommends using Semantic Versioning(http://semver.org/).

A version number below 1.0.0 indicate that your are developing project. (And may break backward compatibility from one version to another without changing Major number in version number) We are currently released as version 0.1.0

If you think that you don't need to make backward incompatible changes frequently, then we should bump up the version number to 1.0.0(I would suggest that we should try to make the package PSR4(http://www.php-fig.org/psr/psr-4/) complaint before releasing version 1.0.0. Making it PSR4 will break backward compatibility.)

If we do however start to use version number 1.0.0, then all version numbers for future releases will need to follow semver conversion.

Major.Minor.Patch

  • Change in Major number indicate break in backward compatibility
  • Change in Minor number indicate new features were added
  • Change is Patch number indicate bug fixes were made

Let me know if you want to increase version number to 1.0.0

@rasodu
Copy link
Collaborator

rasodu commented Aug 12, 2016

Hi @frank-herbert,
You don't need to define repository url in you composer.json since the package is already released on packagist.

You can simply do composer require jumbojett/openid-connect-php:dev-master or add "jumbojett/openid-connect-php": "dev-master" to composer.json install latest code from master branch.

@jumbojett
Copy link
Owner

We've needed versioning for a while. @rasodu I appreciate the initiative. I see this as priority and I don't want to be a bottleneck in this effort. You're welcome to create and accept PRs without my review. What you've described above aligns with what I'm thinking.

Let me know if you want to increase version number to 1.0.0

Yes 👍

(I would suggest that we should try to make the package PSR4(http://www.php-fig.org/psr/psr-4/) complaint before releasing version 1.0.0.

I'm fine with this.

@burnhamrobertp
Copy link

burnhamrobertp commented Feb 28, 2017

How should we go about petitioning for the marking of a new release (in packagist)? Indefinitely using dev-master in composer isn't a viable solution

@kenguest
Copy link
Contributor

kenguest commented May 4, 2017

As @Harkenn as already alluded to, using dev-master in composer for projects using this library on production servers is just asking for serious problems, so could we please get a new release containing the bug fixes and extra functionality since v0.2.0?
Perhaps pulling in extra goodness from the many forks of this project that are out there?

@rasodu
Copy link
Collaborator

rasodu commented May 4, 2017

Version 0.3.0 is released.

We need to add changelog file for for the project(http://keepachangelog.com/en/0.3.0/). @kenguest can you make a pull request to create the file. Also if you can add a note in readme that all the future pull request should add an entry to this file so we know how to increment the version.

@kenguest
Copy link
Contributor

kenguest commented May 4, 2017

Version 0.3.0 is not showing up on packagist [yet] - https://packagist.org/packages/jumbojett/openid-connect-php ...

@rasodu
Copy link
Collaborator

rasodu commented May 4, 2017

Fixed. It is now showing up.

kenguest added a commit to kenguest/OpenID-Connect-PHP that referenced this issue May 4, 2017
kenguest added a commit to kenguest/OpenID-Connect-PHP that referenced this issue May 4, 2017
kenguest added a commit to kenguest/OpenID-Connect-PHP that referenced this issue May 4, 2017
@kenguest
Copy link
Contributor

kenguest commented May 4, 2017

Those multiple commits should not have happened - first time doing a PR through the github website rather than the command line and it seems to have... been a unique experience. Apologies for that.

@rasodu
Copy link
Collaborator

rasodu commented May 9, 2017

I think we need to break #80 into multiple pull request

  • Add chagelog
    • Add CHANGELOG.md
    • Add short notes for developers requesting that pull request should add an entry to CHANGELOG.md if their pull request effects functionality of the project. Either we can add the note to README.md, or we can add new file CONTRIBUTING.md.
    • Add .github/PULL_REQUEST_TEMPLATE.md. This file will contain message that every pull request that changes functionality of the project should add an entry to CHANGELOG.md
  • Add namespace 'Jumbojett\OpenIDConnectPHP'(Or something else) to class OpenIDConnectClient
    • Release version 1.0.0 before adding namespace
    • Add namespace to file 'OpenIDConnectClient.php' and move the file to 'src/OpenIDConnectClient.php'
    • Update composer.json to autoload package classes from 'src/' folder
    • Update code example in README.md to use namespace
    • Release version 2.0.0(At this point anyone wanting to update to this version of the package will be required to add namespace to their code to access the class.)
  • Apply PSR2 coding standard
    • Merge(or close) all pull requests before updating code to PSR2. Any PR that was created before we apply new code formatting will be difficult merge after we update code.
    • Update README.md(OR CONTRIBUTING.md) to tell developer that they should use PSR2 code formatting.
    • Add Travis Job to check that code is PSR2. Once we add Travis integration any pull request will be automatically tested for PSR2 code formatting.

@shadowhand
Copy link

I'm happy to help with this effort. I have a lot of experience with moving packages towards PSR compliance and developing releases. See league/oauth2-client contributions.

@jumbojett
Copy link
Owner

jumbojett commented Jun 2, 2017

@shadowhand I'm all for collaboration!
@kenguest @rasodu can you add @shadowhand to your fork of the repo so he can help with the PRs?

@rasodu
Copy link
Collaborator

rasodu commented Jun 3, 2017

@shadowhand Current class is not namespaced. I think next step should be to add namespace to the class. Can you create a new pull request for this? This will have few ramifications(Mainly BC break). We will discuss them further in the pull request.

@jumbojett @kenguest @shadowhand We need to decide the name we will use for namespace. @shadowhand will need it to kick off the effort. Does anyone have suggestion? We can use 'Jumbojett\OpenIDConnectPHP' or 'Jumbojett\OpenIDConnect'.

@shadowhand
Copy link

I would strongly suggest not using a person as the root namespace. I think PhpOpenId\OpenIdConnect would be perfect.

@jumbojett
Copy link
Owner

@rasodu I vote for Jumbojett\OpenIDConnect simply b/c it would be easier to trace the library back to the source from a composer search. The fact that it's "PHP" is more of an advertisement outside of composer :).

I would strongly suggest not using a person as the root namespace. I think PhpOpenId\OpenIdConnect would be perfect.

@shadowhand could you elaborate? I can see where it would appear more official, but without a github organization or domain, I fear it might mislead.

@shadowhand
Copy link

shadowhand commented Jun 5, 2017

@jumbojett Composer includes a source link on every page, being able to discover the Github page is not a problem.

The problem with using a person as the root namespace is if it gets abandoned, everyone is going to have to update their composer references and there is more friction. Using an organization, even if it currently points to a personal Github page, is safer in the long run because it allows changing the target repository with minimal effort and no effort on the part of users. This is why my package latitude/latitude is not shadowhand/latitude.

Also worth noting that the org phpopenid (or php-open-id if that looks better) is available if someone wants to claim it. ;)

@jumbojett
Copy link
Owner

The problem with using a person as the root namespace is if it gets abandoned, everyone is going to have to update their composer references and there is more friction.

I value your expertise and input @shadowhand. I also understand your reasoning. This repo has been active for almost 5 years. People in the OIDC community link here for support and references. Our library is focused on minimalism. (one single php file) In a way, I feel like not having an organization reflects that. @rasodu let's roll with with the Jumbojett\OpenIDConnect namespace for now unless you have additional input.

@kenguest
Copy link
Contributor

kenguest commented Jun 8, 2017

@jumbojett - as you say, people link. You could simply link from Jumbojett\OpenID-Connect-PHP repo to a superseding repository. Take a look at https://github.com/manuelpichler/phpmd - this is what happened there.

Similarly the official repo for PEAR's Auth_SASL2 repo is at https://github.com/pear/Auth_SASL2 - not the original author's repo at https://github.com/CloCkWeRX/Auth_SASL2.

Links can be updated, redirections can be put in place.

Also if the project does become PSR2 compliant then you won't have just one single PHP file anymore, so your point there is moot to be honest.

@jumbojett
Copy link
Owner

I'd like to make sure we distinguish our library from others. There's already other php OIDC libraries that are PSR2 compliant and more for the individual who wants something that conforms to a standard. (https://github.com/ivan-novakov/php-openid-connect-client)

The goal of this project is readability and minimalism. Less dependencies and less code.

That said...
@kenguest if you and @rasodu the best direction is an org and PSR2, then we'll go that route.

Maybe incorporate some of @kdoyen 's changes / tests as well?
https://github.com/kdoyen/openid-connect-php

@jumbojett
Copy link
Owner

@shadowhand I've been thinking about your proposal. If you would like to take the initiative to create the organization and import the PSR2 compliance work currently in progress then let's run with it.

Steps:

  • Create github org
  • Merge PSR2 work
  • Update versioning
  • Incorporate automated testing for PRs

This project could benefit from your expertise. Feel free to email me directly if you have questions.

@shadowhand
Copy link

shadowhand commented Aug 2, 2017

@jumbojett I dropped the ball here. My interest in working on this package was based on a work need. The project has changed and we will no longer be using OpenID. If something changes in the future, I will happily contribute to this package.

Apologies for the late response. 😞

@jumbojett
Copy link
Owner

jumbojett commented Aug 3, 2017

@shadowhand no worries! Thank you for the update. If something changes, please let us know.

@jumbojett
Copy link
Owner

Inviting @guss77 to the discussion.

@guss77
Copy link
Contributor

guss77 commented Aug 7, 2017

Thanks @jumbojett - I'm happy to be involved. I'm currently maintaining a fork at https://github.com/con-tools/Auth-OpenID-Connect where some PSR-1 was done, in addition to some minor API changes.

I'm interested in completing more PSR-1/2/4 work, before maybe discussing opening the API a bit for extension.

The next thing on @rasodu list is adding the top level \Jumbojett namespace and move the source files into a src directory. I'll go ahead and issue a pull request for that.

This was referenced Aug 7, 2017
@guss77
Copy link
Contributor

guss77 commented Aug 8, 2017

Regarding the merging existing PRs before merging the PSR-2 work, I'm willing to review and fix merge conflicts for any existing PR that there is interest in still completing.

@jumbojett - if you have time to review existing PRs, close what you are not interested in merging and pinging me on any that you are, I'll review and fix what is still open.

@jumbojett
Copy link
Owner

@guss77 I just gave you collaborative access to the repo. You are welcome to review and merge as necessary!

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

No branches or pull requests

8 participants