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

PHP Strict Standards, Notice and Warnings #121

Open
lloc opened this issue Oct 6, 2014 · 15 comments
Open

PHP Strict Standards, Notice and Warnings #121

lloc opened this issue Oct 6, 2014 · 15 comments

Comments

@lloc
Copy link
Contributor

lloc commented Oct 6, 2014

Plugin generates some warnings and notices with PHP 5.5 and WP_DEBUG true when entering the admin screens.

@christianwach
Copy link
Contributor

@lloc You might like to have a look at this branch on my fork:
https://github.com/christianwach/buddypress-courseware/tree/cmw-merged
I merged what I thought were the best bits of the other forks and fixed a whole heap of stuff on top of that. Unfortunately, I had to stop where I did due to time constraints.

@lloc
Copy link
Contributor Author

lloc commented Oct 6, 2014

Ah, I thought that this here might be the "main" repository.

https://twitter.com/jenmylo/status/518744634432831488

@christianwach
Copy link
Contributor

It is the main repo. It's just that @iandunn, @iMadalin and @sboisvert all added great code which has not been merged back in. I built on those forks to produce the branch on my fork. I never opened a PR, though, as this main repo seems unmaintained.

@christianwach
Copy link
Contributor

Oh, I forgot to mention: from my inspection of the forks, it looked like @iandunn rewrote the plugin so that it does not require BuddyPress. It might be that fork which was designed to run on wp.org. I can't remember offhand whether my fork carries that work forward. UPDATE: it does contain the BP-mocking code.

@christianwach
Copy link
Contributor

Either way, my branch will give you a considerable head-start.

@christianwach
Copy link
Contributor

@lloc I just opened a PR so the branch is more easily discoverable.

@Mamaduka
Copy link

Mamaduka commented Oct 6, 2014

I think it would be better, if we won't drop BP as requirement. We might end-up writing a lot of "mock" BP code.

Not sure, if you all saw the tweet from Jen (https://twitter.com/jenmylo/status/518744634432831488), but current priority is to make sure plugin is compatible with latest version of WordPress and possible with latest BP.

Let's stick with this priority for now, we can cherry-pick commits, if any of those does what we need for now.

@christianwach
Copy link
Contributor

@Mamaduka My branch is definitely compatible with BP 1.8, which was the state of play back in June. BP can be dropped as a requirement via a filter, but it's off by default. Happy to feed back on any of my code - if I can remember what it was for :) Look forward to this getting some TLC.

@christianwach
Copy link
Contributor

Unfortunately, it looks like "imadalin" has closed their GitHub account, so all the refactoring that they did on this plugin (basically to update the PHP4-style class declarations and clean up whitespace) no longer exists as a fork. Seems like this commit is the only remaining evidence of it. There was a lot of work done for this.

@stas
Copy link
Member

stas commented Oct 6, 2014

Hey guys, can somebody make a PR we can merge so that everyone can have a common place to start working on?

I know Ian started working on a BPless version, but I'm not sure what is the status of it. He emailed me a couple of months ago with a common issue which I will add as a ticket most likely.

I also know Mădălin started some work, but I never checked what is the status. I believe he's now under @16nsk.
His fork seems to be https://github.com/16nsk/buddypress-courseware

I will be happy to review and merge branches that would first fix existing compatibility issues, after what we could figure out what features could be added/removed.

Thanks a lot for picking up the development.

@christianwach
Copy link
Contributor

Ian Dunn's and Mădălin's forks basically had too many conflicts to maintain the commits from both, so I merged Mădălin's work in manually after applying Ian's commits. My own efforts begin thereafter at 543c7e0 as you can see from the list on #123.

@iandunn
Copy link
Contributor

iandunn commented Oct 6, 2014

I know Ian started working on a BPless version, but I'm not sure what is the status of it. He emailed me a couple of months ago with a common issue which I will add as a ticket most likely.

I haven't worked on it in a long time, but IIRC, it was mostly functional. I think it also had some compatibility fixes. It sounds like Christian has already incorporated it into his fork, but it's at https://github.com/iandunn/buddypress-courseware/tree/bp_not_required for anyone else who wants to take a look.

@lloc
Copy link
Contributor Author

lloc commented Oct 6, 2014

Interesting situation! Seems there is a lot of code... ;) Any ideas how to proceed to reach the first goal

make sure plugin is compatible with latest version of WordPress and possible with latest BP

... can we add all the enhancements after this?

@christianwach
Copy link
Contributor

@lloc I don't remember any of the commits that I inspected (or added) being "enhancements" as such, unless you count BP being optional as an enhancement. IIRC Mădălin's major contribution was to update the classes and introduce the BPSP_Courseware_Component class. The code as I left it in June worked with the then-current version of BP. If you look at the diff between the master and cmw-merged branches, you'll see that there's not a lot in the way of new code - with a few exceptions it's mostly just cleaned up.

@lloc
Copy link
Contributor Author

lloc commented Oct 6, 2014

@christianwach OK, the term enhancement was probably not the best choice. ;) Let us find a pragmatic way to have a starting-point.

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

5 participants