Laravel 9.x Compatibility

[laravel-shift]

This is an automated pull request from Shift to update your package code and dependencies to be compatible with Laravel 9.x.

Before merging, you need to:

• Checkout the l9-compatibility branch
• Review all comments for additional changes
• Thoroughly test your package

If you do find an issue, please report it by commenting on this PR to help improve future automation.

[laravel-shift]

:alembic: Using this package? If you would like to help test these changes or believe them to be compatible, you may update your project to reference this branch.

To do so, temporarily add Shift's fork to the repositories property of your composer.json:

{
    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/laravel-shift/comments.git"
        }
    ]
}

Then update your dependency constraint to reference this branch:

{
    "require": {
        "laravelista/comments": "dev-l9-compatibility",
    }
}

Finally, run: composer update

[AbdallaMohammed]

Can anyone please merge this?

[mabasic]

Can anyone please merge this?

I will merge it as soon as I find the time to review the code.

[FlightSimAssociation]

And update here? Would love to get this merged so I can work on updating my application to Laravel 9.

[mabasic]

And update here? Would love to get this merged so I can work on updating my application to Laravel 9.

You can always fork the repository, apply the changes to your forked version and use that in your project until I deal with this PR.

See composer documentation on how to do this so you don't have to wait for me. There is a simple mechanism just for this exact purpose.

[robbielove]

Thanks for your suggestion.

However, please consider that this approach does not scale well for packages that use this package.

For example - I have made a package that uses this package, if I add the custom repo as described then every other codebase that uses my package will need to have that custom repo added to the composer.json - it will only work (not require custom repo) if you release a branch that contains these changes because then the codebases will be able to 'see' this branch whereas I'll have to add the custom repo to N x codebases where they currently cannot 'see' this branch.

My package is private and only used internally, but you could imagine this would prevent me from releasing my package L9 branch if it was public - everyone who used it would also have to add the custom repo.

This is not your fault - i have to do the same thing for other packages as they all have the same issue but the majority have merged their changes already. I'm simply raising the points;

• that there is a flow-on effect of having to wait (or waste time to a degree if I were to add it to all my codebases)
• that not everyone who uses this package is using it directly; it might be required by another package (people have different use cases and your provided solution helps majority of cases perfectly and others with a solution but perhaps not the best one)
• the impact to each user can be different. I certainly understand this is the easiest approach - but that has a tradeoff of flexibility and the more time it takes to complete this release cycle exacerbates that.

This means it becomes too time-consuming to test this across all codebases. I will have to remove the custom repo and update it again when this changes. I would run out of time if I had to do this for all packages. (I'll just have to wait)

Instead of proposing others fork it - why not just change the base of this PR to be a branch of the same name (and merge it) - that way we can simply change the branch in composer (which works for packages that require this package) but aren't required to add a custom vcs repo in composer for each one. Another PR can be raised from the local l9-compatibility branch to master. Then once you are happy you can simply merge that branch into master.

eg. this should work - because the branch would exist in this repo(without needing to specify a fork)

"laravelista/comments": "dev-l9-compatibility",

then any codebases using packages that require "laravelista/comments": "dev-l9-compatibility" would be able to resolve that branch without requiring custom vcs config and can simply update the dependency when the PR into master is merged (or whatever branch)

I understand if you choose not to do this, I appreciate your contributions to the package.

[mabasic]

Thanks for your suggestion.

However, please consider that this approach does not scale well for packages that use this package.

For example - I have made a package that uses this package, if I add the custom repo as described then every other codebase that uses my package will need to have that custom repo added to the composer.json - it will only work (not require custom repo) if you release a branch that contains these changes because then the codebases will be able to 'see' this branch whereas I'll have to add the custom repo to N x codebases where they currently cannot 'see' this branch.

My package is private and only used internally, but you could imagine this would prevent me from releasing my package L9 branch if it was public - everyone who used it would also have to add the custom repo.

This is not your fault - i have to do the same thing for other packages as they all have the same issue but the majority have merged their changes already. I'm simply raising the points;

• that there is a flow-on effect of having to wait (or waste time to a degree if I were to add it to all my codebases)
• that not everyone who uses this package is using it directly; it might be required by another package (people have different use cases and your provided solution helps majority of cases perfectly and others with a solution but perhaps not the best one)
• the impact to each user can be different. I certainly understand this is the easiest approach - but that has a tradeoff of flexibility and the more time it takes to complete this release cycle exacerbates that.

This means it becomes too time-consuming to test this across all codebases. I will have to remove the custom repo and update it again when this changes. I would run out of time if I had to do this for all packages. (I'll just have to wait)

Instead of proposing others fork it - why not just change the base of this PR to be a branch of the same name (and merge it) - that way we can simply change the branch in composer (which works for packages that require this package) but aren't required to add a custom vcs repo in composer for each one. Another PR can be raised from the local l9-compatibility branch to master. Then once you are happy you can simply merge that branch into master.

eg. this should work - because the branch would exist in this repo(without needing to specify a fork)

"laravelista/comments": "dev-l9-compatibility",

then any codebases using packages that require "laravelista/comments": "dev-l9-compatibility" would be able to resolve that branch without requiring custom vcs config and can simply update the dependency when the PR into master is merged (or whatever branch)

I understand if you choose not to do this, I appreciate your contributions to the package.

Only the package that uses my package would have to add the custom vcs (your forked version). Other packages that require your package would not need to add the custom vcs. My package is public. Your fork of my package should be public as well. Your package that uses your forked version of my package can be private. It should all work.

You can release a version of your package with this custom cvs and your fork of my package. Then when I merge this PR you can (if you want, you don't have to) release a new version in which you stop using your fork.

There is no overhead on your part. You fork, make changes, add custom vcs, release version and forget about it until the next version.

See here for more details: https://medium.com/swlh/using-your-own-forks-with-composer-699358db05d9

This is my understanding of things, maybe I am wrong. I have used this in the past but can't remember the limitations, if any.

[robbielove]

• yes I realise that I can make a fork
• each other person with the same issue will also have to to this
• I don't want to make a fork for a number of reasons
• I'm well aware of how to use forks with composer (I already do this for packages that don't want to support uuid's as the primary key for their tables)
• I don't think that you understand that I am referring specifically to adding the laravel shift fork (which is basically the same as me forking it)

suppose i made a simple package that just requires this one:

{
    "name": "example/package",
    "description": "Example package",
    "require": {
        "laravelista/comments": "^4.4",
    }
}

no problems here - make a new laravel app and composer require example/package - it should work.

I see this PR and decide to try it out on my package: all seems fine so far (I could install it with composer install - but in order to use it i would have to require this example package in a laravel app)

{
    "name": "example/package",
    "description": "Example package",
    "require": {
        "laravelista/comments": "dev-l9-compatibility",
    },
    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/laravel-shift/comments.git"
        }
    ]
}

so lets say i made a new laravel app: here lies the problem - it can not find this dev-l9-compatibility branch: (because it does not know about the laravel shift fork)

{
    "name": "new/laravel",
    "description": "New Laravel App",
    "require": {
        "example/package": "*",
    }
}

unless I also add the laravel shift fork:

{
    "name": "new/laravel",
    "description": "New Laravel App",
    "require": {
        "example/package": "*",
    },
    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/laravel-shift/comments.git"
        }
    ]
}

if I have 100x apps - I have to do this 100x (As does every other dev using it like this) where as if you change this PR's base to a branch (that you create) called l9-compatibility and merge this PR into it - the changes will be available to all without requiring the laravel shift (or their own) fork. and it will not affect the current release as these change are isolated to a separate branch.

Then we can raise a PR for laravelista:l9-compatibility and you can test it and take as much time as needed to test and eventually merge when done.

this means that everyone wins - those using a release will continue to use that version with no issues

• those who want bleeding edge can simply use the laravelista:l9-compatibility branch (including those where its used by a package that they use)
• those using dev-master will get the changes when the PR is merged

I know this is the case because my forked packages are private and I already have to do this for them. now thats not as much of an issue as I added them once and they wont change because I'll use those forks forever. but in this case I am trying to avoid chopping and changing the custom repos just to test out a version change that will likely be merged and then ill have to change them all again, and this will repeat every time a new version comes out (L10, L11, etc) - it gets out of hand.

To show you: this is what happens when I try to update my laravel app that uses my package (which in turn uses comments package):

Loading composer repositories with package information                                                                Updating dependencies                                 
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Root composer.json requires example/package dev-laravel/9 -> satisfiable by example/package[dev-laravel/9].
    - example/package dev-laravel/9 requires laravelista/comments dev-l9-compatibility -> found laravelista/comments[dev-l9-compatibility] in the lock file and laravelista/comments[dev-feature/likes, dev-master, dev-develop, 1.0.0, 1.1.0, 1.1.1, 1.1.2, 2.0.0, ..., 2.1.0, 3.0.0, ..., 3.6.1, 4.0.0, ..., 4.5.0] from composer repo (https://repo.packagist.org) but these do not match your constraint and are therefore not installable. Make sure you either fix the constraint or avoid updating this package to keep the one from the lock file.

[mabasic]

In my previous link you can see how to tell composer to use that forked version as a specific version so that you don't get that message.

I understand what you want, but the downside to that is that I will eventually delete that branch on my repo and your packages will throw errors on install. Then you would have to update them regardless.

I do not want to create a new branch on my repo for Laravel 9. If anywhere it should go in the master branch... that way you can use the master branch since I use that branch for unstable things, but I would not recommend doing that.

My recommendation is to use the approach that I've suggested (forked version. This is what I would do in your case) or wait (if you can). But, if you want I can merge this PR to master and you can use that?

The benefit of using the forked version is that you are in charge of when the changes happen. The downside of using the master branch is that it is unstable (as is this PR until I test it) and you rely on me.

The reason why it takes me this long to merge this PR is that I don't have a single L9 project yet. I work on L5.8 to L8 projects. I would have to read about L9, install it, try using this package and if any fix bugs. Switch to PHP 8.1 or 8.2 which ever is latest etc.

I don't have time for this atm. If I were to use this package in a L9 app I would fix it asap, but as things are now I have other projects that I need to finish first that bring me money.

[robbielove]

Thanks for explaining.

I would likely pin my version at a later date once you tag a release so changing branches for me is not an issue as it is less work because I only have to do it per package not per app. However I personally prefer not to delete things including branches because; as you say- they would stop working in the future, as would the url to the branch on github.

This guided me to form the opinion merging master is best.

I would also be interested if you would mind explaining aside from the L9 changes, how unstable or different is master compared with the latest stable release, and how likely are you in the future to pollute it with changes?

Assuming now I understand how you use this branch; I can manage with that in mind. I need this package to work for my package so I would try to fix issues if they arise and raise PR's - hopefully this would keep the instability to a minimum.

[robbielove]

https://github.com/laravelista/comments/compare/4.5.0...master These changes seem fine to me, I'm happy to merge this (L9) branch into master. If you wouldn't mind, please.

[mabasic]

@robbielove I have merged the branch to master.

The master branch is very stable but can be totally unstable when I actively work on things. Considering how much or how little time I have been working on this repository I think that it is safe to use.

PRs for fixes to the main branch are welcome. Let me know if the package works on Laravel 9.

[Jipem]

@mabasic : Before I was with Laravel 8 and it was ok (laravelista/comments 4.3.0). I just switched to Laravel 9 and I have a problem with this merge. When validating a comment, I get the error: Target [Laravelista\Comments\CommentController] is not instantiable. 😔

[mabasic]

@Jipem No "official" support for Laravel 9 yet, hopefully during the summer vacation I will work on this package and fix this.

[Jipem]

@mabasic Thanks, if it helps (when you have time), I have no more problems with : #193

[mabasic]

@mabasic Thanks, if it helps (when you have time), I have no more problems with : #193

@Jipem Thank you for your contribution. I have merged it to master.