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

Use official PSR-15 interfaces #15

Closed
wants to merge 1 commit into from

Conversation

simoheinonen
Copy link

@mindplay-dk
Copy link
Owner

Thank you, but per #17, I can't merge this to master, since I will need to maintain the 3.x version long term.

I'll see about creating a 4.x branch and merging this as soon as possible!

@rlweb
Copy link

rlweb commented Apr 27, 2018

@mindplay-dk it'll be great to get this in, I found your package really simple to use but to get it working with other middleware was not so easy!

@mindplay-dk
Copy link
Owner

@rlweb I'm not sure what you mean? PSR-15 has been supported since 3.0. We merely can't remove BC with the legacy interfaces.

What was the difficulty?

@gugglegum
Copy link

@mindplay-dk The difficulty is in that 3.0.2 version uses non-official packages from "http-interop". If you use middlewares which implements official PSR interfaces, your dispatcher throws an exception "unsupported middleware type". And this is really not easy to make friends two different interfaces.

@gugglegum
Copy link

@rlweb I found a nice temporary solution for you. Thanks to @simoheinonen for his fork and branch. Try this in your root composer.json. At first, add @simoheinonen's repository as his repo contains fixed version:

    "repositories": [
        {
            "type":"vcs",
            "url":"git@github.com:simoheinonen/middleman.git"
        }
    ],

Then in "require" section add mindplay/middleman package with specified branch version containing fix:

    "require": {
        "mindplay/middleman": "dev-psr-15 as 3.0.2.1"
    }

Then remove folder vendor/mindplay/middleman and run composer update. This will allow you to use this package with official PSR interfaces like Psr\Http\Server\* until @mindplay-dk will release 4.0 version for this package with official PSR support from the box.

Thanks to @simoheinonen once again, you're awesome.

@mindplay-dk
Copy link
Owner

If you use middlewares which implements official PSR interfaces, your dispatcher throws an exception "unsupported middleware type"

Huh?

There's a test demonstrating support for the official PSR interface - it passes.

So I'm not sure what you mean.

this is really not easy to make friends two different interfaces

Yes, it is - the old interfaces were updated after the final release; the old middleware interface is just an empty interface that extends the new official interface.

For middleman, this means it supports both the old and new interface.

You can implement the new interface and just ignore the fact that the old interface exists.

@mindplay-dk
Copy link
Owner

Okay, so it looks like they messed me up back in January - I was never aware of that.

Unfortunately, simply switching to the official interfaces still isn't an option, so I'm trying to come up with a backwards-compatible fix...

@mindplay-dk
Copy link
Owner

@simoheinonen @rlweb @gugglegum I've pushed an unstable 3.0.3 branch, if you'd like to test.

It worked when I upgraded one of our own middleware-heavy projects, but the whole thing is really confusing, and made even more confusing by the recent aliasing and hacks for forward compatibility in the legacy packages, the split into two packages, etc. so I'm a bit uncertain about this 🙄

@gugglegum
Copy link

@mindplay-dk I tested 3.0.3 branch and it's working for me well. For other people who don't know how to install a non-released version from branch -- add it as follows:

{
    "require": {
        "mindplay/middleman": "3.0.3.x-dev"
    }
}

@mindplay-dk
Copy link
Owner

You may need to add "minimum-stability": "dev" as well, to allow installation of unstable packages.

@gugglegum
Copy link

gugglegum commented May 7, 2018

@mindplay-dk It may need minimum-stability if you use it in another composer package. But if you add this package in root composer.json, you don't need to change minimum stability as "dev" version already specified in package version explicitly. Here's a real working composer.json of my test application:

{
    "autoload": {
        "psr-4": {
            "": ""
        }
    },
    "require": {
        "aura/router": "^3.0",
        "vlucas/phpdotenv": "^2.4",
        "luracast/config": "^2.0",
        "middlewares/aura-router": "^1.0",
        "middlewares/request-handler": "^1.1",
        "zendframework/zend-diactoros": "^1.0",
        "mindplay/middleman": "3.0.3.x-dev"
    }
}

@mindplay-dk
Copy link
Owner

@simoheinonen would you review this change and see if it fully addresses what you were trying to address with your PR? (I took your cue on documentation changes, etc.)

@rlweb can you let me know if this alleviates your problems?

@rlweb
Copy link

rlweb commented May 8, 2018

@mindplay-dk this is great and works well! Thanks

@mindplay-dk
Copy link
Owner

Closing in favor of #19

@mindplay-dk mindplay-dk closed this May 9, 2018
@mindplay-dk
Copy link
Owner

Thanks for your feedback, guys! 3.0.3 has been tagged :-)

@gugglegum
Copy link

Still works as expected. We are waiting for 4.0 version without http-interop/http-server-middleware. It may be all the same just without backward compatibility.

@mindplay-dk
Copy link
Owner

We are waiting for 4.0 version without http-interop/http-server-middleware

I think there's no reason to wait for the removal of a feature - we have everything we need now, and can move forward.

I don't expect there will be a 4.0 release until there's a real reason to make a breaking change - at that point, I will see the 4.0 release as an opportunity to remove support for legacy middleware, but at this time I'm not going to publish a major release just for that.

@gugglegum
Copy link

gugglegum commented May 11, 2018

@mindplay-dk Why not? Do you pity the numbers? Look at Windows and iPhones — they even skip major numbers. Look at PHP that skipped 6 major version. MySQL skipped 6 and 7 version, MariaDB… Do not pity the numbers, they are worthless. The reason to make new major version is to eliminate warning about using abandoned package on every composer update.

@mindplay-dk
Copy link
Owner

Why not?

Major upgrades are a hassle with PHP and Composer, where major version increments lead to conflicts - if this were JS and NPM, I wouldn't think twice, but I don't want to create an upgrade hassle without a good reason. Removing a feature purely for aesthetics is not a good enough reason.

It's a "nice to have", not a "must have", so it's simply not a priority atm.

@mindplay-dk
Copy link
Owner

...if it had been a priority, and if I didn't care about backwards compatibility, I would have simply pressed "merge" on @simoheinonen's PR 😄

Repository owner locked as resolved and limited conversation to collaborators May 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants