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

Code cleanup (WIP) #274

Merged
merged 53 commits into from
Dec 13, 2016
Merged

Code cleanup (WIP) #274

merged 53 commits into from
Dec 13, 2016

Conversation

mbirth
Copy link
Contributor

@mbirth mbirth commented Jun 30, 2016

This is a working snapshot of a complete overhaul of the COPS codebase with your merges from today included.

Most packages are now external libs and pulled in by Composer (and its Autoloader). Scrutinizer checks code formatting (Symfony style). No more multiple classes in one PHP file. And some code fixes/cleanups (more to come).

No functional changes were made to the code.

Builds fine in my Travis and Scrutinizer.

After merging, you have to fix the links in the README to point to your badges again.

mbirth added 30 commits June 11, 2015 00:36
@seblucas
Copy link
Owner

That's a huge Pull request to check ! Thanks;).

I checked many files and I'm mostly ok. One thing that bug me is the modification of the EOL character in the language file. A large number of users still use Windows's Notepad (so LF is big no go !) and I want those files to be really easy to change (In addition there's less noise in the files changed tab).

@mbirth
Copy link
Contributor Author

mbirth commented Jun 30, 2016

I was thinking about using some online service like Crowdin, later on. Then users can translate there and you just pull the data files into the project. Then it's no problem anymore.

But for now, I just changed the line endings of the translations back to CRLF.

@seblucas
Copy link
Owner

@horus68 has already set up transifex but I did not spent enough time on it to enable integration with Github

@mbirth
Copy link
Contributor Author

mbirth commented Jul 3, 2016

I just merged your most recent commits into the PR.

@seblucas
Copy link
Owner

I tested your branch and did not see any regression for now using PHP 5.4 / 5.5

With PHP 7 there is a problem with $config :

`Undefined variable: config in C:\Projets\cops-mbirth\base.php on line 13

I'll continue to test till the weekend (I installed your branch on my main library) and will merge it on monday if everything goes well

@mbirth
Copy link
Contributor Author

mbirth commented Sep 30, 2016

Were there any further problems with this branch?

@seblucas
Copy link
Owner

seblucas commented Oct 3, 2016

Besides the problem with PHP7, I don't think I found anything. I tried to fix the PHP7 problem in early august but I didn't understood the problem

@mbirth
Copy link
Contributor Author

mbirth commented Oct 23, 2016

I just merged all recent changes into this branch. I'll try it with PHP 7.0 soon and hopefully fix that last bug.

@seblucas
Copy link
Owner

Thanks a lot, I won't merge anything else in the meantime to avoid complicating your task even more :(.

@mbirth
Copy link
Contributor Author

mbirth commented Oct 25, 2016

It seems PHP 7 ignores require_once even if the previous loading of that file was in a different context. I changed all loadings of config files to require and now it works with PHP 7.

I also had to change the config loading order for the "live" config to not overwrite the "test" config.

Could you try it again, please?

@seblucas seblucas merged commit 704d466 into seblucas:master Dec 13, 2016
@seblucas
Copy link
Owner

Thanks a lot .... especially for your patience :).

@horus68
Copy link
Contributor

horus68 commented Dec 13, 2016

Note: There has been a change in EOL from previous master to the merged files (was Windows, now Unix).
Is this going to be the new standard for the future?
I'm ok with both, I just think its good to have same EOL in all project.

@seblucas
Copy link
Owner

seblucas commented Dec 13, 2016

I know but this was the oldest PR and the cleanup was needed.

With this PR, everything except languages files should use LF.

@seblucas
Copy link
Owner

I'll try to see what I can do for the existing PRs

@seblucas
Copy link
Owner

@mbirth

It would be great if you could have a look on the changes I had to make on composer.json / lock to make it work fine. A second pair of eye would reassure me.

Saucelabs also give me trouble but I'll handle that.

@mbirth
Copy link
Contributor Author

mbirth commented Dec 14, 2016

@seblucas Sure, I'll have an eye on it, but probably not before the weekend. But I'll stay in touch.

dunxd pushed a commit to dunxd/mikespub-cops that referenced this pull request Sep 10, 2023
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

Successfully merging this pull request may close these issues.

3 participants