search

sonata-project / cache

[Deprecated] Cache library
https://github.com/sonata-project/cache
MIT License
331 stars 29 forks source link

Reordered composer requirements #80

Closed jordisala1991 closed 7 years ago

jordisala1991 commented 7 years ago

I am targeting this branch, because this applies for this new release. Also this could be done for 1.x so maybe we need to move this PR to 1.x branch first.

Changelog

### Fixed
- Add PHP and Psr/log as required dependencies

Subject

At first I wanted to update PHP from 7.0 to 7.1, then I saw the other things:

greg0ire commented 7 years ago

You should have done one commit per item in your list, using git add -p. Also "reored" => "reordered"

greg0ire commented 7 years ago

psr/log was introduced in 7e09191a102143164294454abe1e92c65363a9da, and no reason was given for doing that. I think it is safe to move it to the require section without giving it too much thought though, because it's a very common and lightweight dependency to have.

greg0ire commented 7 years ago

Remove autoload-dev (was removed on AdminBundle a long ago)

Please explain why you did that in the commit message. I think it is because phpunit does not need autoload to work.

jordisala1991 commented 7 years ago

Will re-do my commits in a better way asap

jordisala1991 commented 7 years ago

Now the commits should be better, please review @sonata-project/contributors

greg0ire commented 7 years ago

The build fails though. Autoload is needed if you extend classes, unless you require them manually

jordisala1991 commented 7 years ago

You're right, we can remove autoload-dev only if autoload already covers those directories. On this repository we are using src / tests structure which is different from the Sonata Bundles.

Removed that commit, not it should pass.

Also added some phpunit.xml.dist modifications just to see if it changes something on the coverage report (we can move it to other PR on the 1.x branch if we like the change)

greg0ire commented 7 years ago

we can remove autoload-dev only if autoload already covers those directories

No, the right rule is remove it if you don't need to autoload any test class. Here, we extend a test class BaseTest or whatever, and we need to autoload it. If there was no base test class, no autoload would work just fine.

OskarStark commented 7 years ago

Thank you @jordisala1991 👍