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

PSR 2/4 Compliance #150

Closed
jaswrks opened this Issue Oct 28, 2015 · 23 comments

Comments

Projects
None yet
2 participants
@jaswrks
Member

jaswrks commented Oct 28, 2015

  • The entire codebase should be formatted for PSR-2 compliance.
  • Instead of using Git Submodules, we should switch to Composer.
  • Convert to Phing build script if possible.
  • Consider building the lite variation from the pro version like ZC.
  • Resolve this bug whenever reformatting and cleanup takes place.
  • Partially Fully resolve this issue.

Remaining...

  • Finish refactoring classes.
  • Review :: static calls and refactor.
  • Review underscore calls and refactor.
  • Search/replace .exit( concatenation calls.
  • Lint check entire codebase for PSR 2/4 issues.
  • Add conflict check; i.e., CM vs. CM Pro back in.
  • Search for (?:static|cache)Key(' with static references.
  • Review and perhaps remove several calls to (?:include|require)(_once)?
  • Review SQL queries with single quotes, which previously had double quotes.

Remaining...

  • Add /*[pro strip-from="lite"]*/ /*[/pro]*/ markers for lite generation.

    (partial completion)


Noting that existing custom templates out-in-the-wild will be broken by these changes. Simple templates will remain functional, but Advanced PHP templates that have been customized in previous versions will no longer work after these changes.

@jaswrks jaswrks added the enhancement label Oct 28, 2015

@jaswrks jaswrks referenced this issue Oct 28, 2015

Closed

Feature/149 #23

@jaswrks jaswrks self-assigned this Oct 28, 2015

@jaswrks

This comment has been minimized.

Member

jaswrks commented Oct 28, 2015

Assigning this issue to myself. This needs to be done sooner rather than later; if I can find the time. At the moment, we are all juggling both the old and the new formatting styles and there is no easy way for us to all get on the same page when it comes to the Comment Mail codebase.

jaswrks pushed a commit to websharks/comment-mail-pro that referenced this issue Dec 2, 2015

@jaswrks jaswrks referenced this issue Dec 2, 2015

Closed

PR: feature/150 #33

jaswrks pushed a commit that referenced this issue Dec 2, 2015

@jaswrks jaswrks referenced this issue Dec 2, 2015

Closed

PR: feature/150 #167

@raamdev

This comment has been minimized.

Contributor

raamdev commented Dec 2, 2015

@jaswsinc Do you think we can close this issue in time for the RC (Friday, December 4th) or should I punt this to a Future Release milestone?

@jaswrks

This comment has been minimized.

Member

jaswrks commented Dec 2, 2015

I would punt this. I'm working on this, but even if I get it done before Friday it should go through a lot more testing than we have time for in the upcoming RC IMO.

@jaswrks

This comment has been minimized.

Member

jaswrks commented Dec 2, 2015

I should note that I will probably push some work on this together with my review and tweaks to the MailChimp integration. However, I plan to leave this open and continue working on this for the next couple weeks.

@raamdev

This comment has been minimized.

Contributor

raamdev commented Dec 2, 2015

Copy that. Thank you.

@jaswrks

This comment has been minimized.

Member

jaswrks commented Jan 26, 2016

@raamdev The work in this issue is becoming slightly more urgent.

However, I'm concerned about trying to tackle this while everyone has open PRs, because this issue is going to introduce sweeping changes (e.g., formatting indentation, brackets, etcs), potentially causing a lot of confusion with open PRs that you are trying to merge in. I suggest that we try to schedule a week for this work to be done, and during that week we try to make sure there are no PRs open already, and that everyone works on something else while I work on this issue.

@jaswrks

This comment has been minimized.

Member

jaswrks commented Jan 27, 2016

You have the next release scheduled for Feb 2nd, so I suggest we try to start work on this Feb 5th (the following Friday). I might be able to do it over that weekend, and if not I'll have the rest of the following week to finish it up. I'm adding this to the calendar so that you can OK it.

@raamdev

This comment has been minimized.

Contributor

raamdev commented Jan 27, 2016

next release scheduled for Feb 2nd, so I suggest we try to start work on this Feb 5th (the following Friday)

Agreed. I'm assigning this to the Future Release milestone so that we can tackle it first-thing as after the current release is finished.

@raamdev raamdev added this to the Future Release milestone Jan 27, 2016

@jaswrks

This comment has been minimized.

Member

jaswrks commented Feb 2, 2016

Noting that this was pushed to Feb 12th by Raam.

@jaswrks

This comment has been minimized.

Member

jaswrks commented Feb 8, 2016

Adding new item to the checklist above.

  • Look for this bug whenever reformatting and cleanup takes place.
@raamdev

This comment has been minimized.

Contributor

raamdev commented Feb 12, 2016

@jaswsinc FYI, I assigned issues #173 and #230 to you, since you want to work on those as part of this GitHub issue.

jaswrks pushed a commit to websharks/comment-mail-pro that referenced this issue Feb 12, 2016

@jaswrks jaswrks referenced this issue Feb 12, 2016

Closed

PR: feature/150 #56

jaswrks pushed a commit that referenced this issue Feb 12, 2016

@jaswrks jaswrks referenced this issue Feb 12, 2016

Closed

PR: feature/150 #233

@raamdev

This comment has been minimized.

Contributor

raamdev commented Feb 13, 2016

@jaswsinc This work can proceed now that the Comment Mail v160213 GA release has been completed. Let me know if you'd like help with anything here. 😄

@jaswrks

This comment has been minimized.

Member

jaswrks commented Feb 13, 2016

Awesome. Thanks!

jaswrks pushed a commit to websharks/comment-mail-pro that referenced this issue Feb 13, 2016

@jaswrks jaswrks referenced this issue Feb 13, 2016

Closed

PR: feature/150 #58

@jaswrks

This comment has been minimized.

Member

jaswrks commented Feb 15, 2016

Noting that existing custom templates out-in-the-wild will be broken by these changes. Simple templates will remain functional, but Advanced PHP templates that have been customized in previous versions will no longer work after these changes.

@jaswrks

This comment has been minimized.

Member

jaswrks commented Feb 15, 2016

New Namespace Constants (like Comet Cache)

cc @kristineds

define(__NAMESPACE__.'\\QV_PREFIX', 'cm_');
define(__NAMESPACE__.'\\SHORT_NAME', 'CM');
define(__NAMESPACE__.'\\NAME', 'Comment Mail');
define(__NAMESPACE__.'\\DOMAIN', 'comment-mail.com');
define(__NAMESPACE__.'\\GLOBAL_NS', 'comment_mail');
define(__NAMESPACE__.'\\SLUG_TD', 'comment-mail');
define(__NAMESPACE__.'\\TRANSIENT_PREFIX', 'cmtmail_');
define(__NAMESPACE__.'\\VERSION', ${__FILE__}['version']);
define(__NAMESPACE__.'\\PLUGIN_FILE', ${__FILE__}['plugin']);
define(__NAMESPACE__.'\\IS_PRO', ${__FILE__}['is_pro']);

Comment Mail has two that Comet Cache does not have yet.

  • QV_PREFIX
  • TRANSIENT_PREFIX
@raamdev

This comment has been minimized.

Contributor

raamdev commented Feb 15, 2016

@jaswsinc writes...

Noting that existing custom templates out-in-the-wild will be broken by these changes. Simple templates will remain functional, but Advanced PHP templates that have been customized in previous versions will no longer work after these changes.

Hmm, that seems like a pretty big deal. What sort of failure will occur if someone did customize an Advanced PHP template in a prior release? Is there any way for us to detect if Advanced PHP templates have been modified and notify the site owner that they need to update their template?

@jaswrks jaswrks referenced this issue Feb 15, 2016

Closed

Build Lite From Pro Codebase #237

1 of 1 task complete
@jaswrks

This comment has been minimized.

Member

jaswrks commented Feb 17, 2016

What sort of failure will occur if someone did customize an Advanced PHP template in a prior release?

They use a different PHP namespace and they call upon methods that were renamed to their camelCase equivalent. So using older PHP templates with the newly modified codebase will result in PHP fatal errors.

Is there any way for us to detect if Advanced PHP templates have been modified and notify the site owner that they need to update their template?

A version-specific upgrade routine might be able to handle this.

@raamdev

This comment has been minimized.

Contributor

raamdev commented Feb 18, 2016

A version-specific upgrade routine might be able to handle this.

Roger that. I think we should definitely work on this to avoid fatal PHP errors if at all possible. I'll open a separate issue to track this.

@raamdev

This comment has been minimized.

Contributor

raamdev commented Feb 18, 2016

@jaswsinc I'm ready to merge this into 000000-dev unless you have any objections. The remaining work can be worked on in #237 and #238 after we merge into 000000-dev.

@jaswrks

This comment has been minimized.

Member

jaswrks commented Feb 18, 2016

No objections :-)

@raamdev

This comment has been minimized.

Contributor

raamdev commented Feb 18, 2016

Next Lite Release Changelog:

  • Restructured Codebase: The codebase has been completely restructured to improve performance, enhance flexibility, and make it easier to build in new features! Props @jaswsinc. See Issue #150.
@raamdev

This comment has been minimized.

Contributor

raamdev commented Feb 18, 2016

Next Pro Release Changelog:

  • Restructured Codebase: The codebase has been completely restructured to improve performance, enhance flexibility, and make it easier to build in new features! Props @jaswsinc. See Issue #150.

@raamdev raamdev closed this Feb 18, 2016

raamdev added a commit that referenced this issue Jun 18, 2016

Phing release of v160618 with the following changes:
- **Restructured Codebase**: The codebase has been completely restructured to improve performance, enhance flexibility, and make it easier to build in new features! Props @jaswsinc. See [Issue #150](#150).
- **Comment Mail Pro Upgrade Notice: Incompatible Advanced Templates.** This version of Comment Mail includes a rewritten and improved codebase. This rewrite, however, came with the unfortunate side effect of breaking backwards compatibility with Advanced Templates that were customized in a previous version of Comment Mail Pro.

     If you are currently using Comment Mail Pro and you've customized your Advanced Templates, all of your customized Advanced Templates will be backed up and the templates will then be reset to their new defaults. You will find the backup of your old customized template appended to the bottom of the new template, separated with a  <code>Legacy Template Backup</code> PHP comment. See [example screenshots](#238 (comment)).

     Note: This change has no effect on Simple templates—only Advanced Templates are affected. Advanced Templates are a Pro-only feature, so this notice only applies to Comment Mail Pro. See [Issue #238](#238).
- **Bug Fix**: Fixed a bug where `esc_html()` was being used where `esc_sql()` should've been used. Props @jaswsinc @kristineds. See [Issue #268](#268).
- **Bug Fix**: Fixed a bug that in some scenarios resulted in a "DB table creation failure" error when activating the plugin. Props @thienhaxanh2405, @PanNovak, @kristineds, and @jaswsinc. See [Issue #260](#260).
- **Bug Fix**: Fixed a bug where "New reply" notification emails were not being parsed properly by some Hotmail accounts and were showing up as blank. Props @kristineds. See [Issue #259](#259).
- **Bug Fix**: Fixed a bug that allowed spam comments to create subscriptions in Comment Mail when using Akismet. Props @IvanRF. See [Issue #250](#250).
- **Bug Fix** (Pro): When Chrome or Firefox Autofill Username/Password was enabled, the Comment Mail Pro Updater fields would incorrectly be autofilled by the browser with invalid credentials. This has been fixed. Props @renzms. [Issue #274](#274).
- **Bug Fix**: Fixed a bug where the cron job for the Queue Processor could get deleted and never recreated, which would result in notifications getting stuck in the Mail Queue and not being sent out. If you ever installed Comment Mail and then deleted it (without first disabling Data Safeguards), and then installed Comment Mail again, you were probably affected by this issue. This release fixes the issue and makes the cron setup more robust. Props @kristineds, @renzms, @jaswsinc, and @IvanRF for help testing. See [Issue #194](#194) and [Issue #173](#173).
- **Bug Fix:** Fixed a bug where a subscriber who selected Hourly Digest and who had never been notified before could, in some scenarios, have their subscription treated instead as a Weekly Digest. This bug was found and fixed during the codebase restructuring. Props @jaswsinc. See [Issue #150](#150) and additional discussion in [Issue #173](#173 (comment)).
- **Bug Fix:** Fixed a bug where in some scenarios Mail Queue entries for Digest Notifications that should have been held for sending later were not being held and were also not being sent. They also would not have shown up in the Mail Queue Event Log. This bug was found and fixed during the codebase restructuring. Props @jaswsinc. See [Issue #150](#150) and additional discussion in [Issue #173](#173 (comment)).
- **Enhancement**: Minor improvements to the Options Page menu links and positioning of the Pro Preview link. Props @renzms. See [Issue #227](#227).
- **Enhancement**: It's now possible to use the following shortcodes in the Email Footer Tag for Email Footer Templates: `[home_url]`, `[blog_name_clip]`, and `[current_host_path]`. Props @kristineds and @IvanRF. See [Issue #246](#246).
- **Enhancement**: Improved the Subscriptions meta box that appears on the Post Edit screen. For each subscription, the meta box now lists the full name and email address, the date the subscription was created, and a view link that allows you to view/edit the subscription. Props @kristineds. See [Issue #231](#231).
- **UX Enhancement (Pro)**: Improved the Dashboard notice that appears when you try to enable the Pro version of Comment Mail when the Lite version is currently enabled. Props @kristineds @jaswsinc. See [Issue #230](#230).
- **UX Enhancement**: When Subscribing Without Commenting, the Add New Subscription form now pre-populates the Name and Email address fields whenever possible. Props @kristineds. See [Issue #204](#204).
- **UI Enhancement**: Dashboard notices generated by Comment Mail now use the WordPress-style dismiss button to keep things consistent. Props @kristineds. See [Issue #193](#193).
@raamdev

This comment has been minimized.

Contributor

raamdev commented Jun 19, 2016

Comment Mail v160618 has been released and includes changes from this GitHub Issue. See the v160618 announcement for further details.


This issue will now be locked to further updates. If you have something to add related to this GitHub Issue, please open a new GitHub Issue and reference this one (#150).

@websharks websharks locked and limited conversation to collaborators Jun 19, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.