-
Notifications
You must be signed in to change notification settings - Fork 3
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
Version Upgrade Routine for customized Advanced PHP templates in version <= v160213 #238
Comments
@jaswsinc I could use some help here if you have some time over the next few days. |
Sorry. I'm not seeing an easy way to deal with this issue. The only way for us to provide full back compat for the old advanced PHP templates would be to restore all sorts of old class method names and also put those under the older namespace too. That doesn't seem feasible to me. There were a lot of changes. Even if we wanted to do that, I think it would be very prone to errors. What we could do instead is to detect a site that has altered the advanced templates in some way, and if that's the case, save that data before we update their options, and then provide a warning/message. ... Hmm, that seems problematic also. I'll give it some thought. |
Another option would to do an interim release (i.e., using the old codebase/structure) and add a routine to the automatic updater that will check if the site owner has customized the advanced PHP template files. If they have, the pro update message could inform them of the situation before they do the upgrade. |
That sounds like the best option here. So would that just be a Dashboard message that tells them the next update will revert their customized Advanced Templates? Or is there something else we can do to make this transition more smooth (i.e., since we'd be releasing a version that is compatible with their current advanced templates, is there anything else we could do)? |
@jaswsinc Ping. ↑ |
@jaswsinc Ping. ↑↑ |
Thanks for the ping. I was thinking about it being a notice that would magically detect if they are using advanced templates That notice could then be displayed whenever an upgrade of the pro version is about to take place. Instead of doing the upgrade, we redirect and ask them to save any advanced template customizations, and then empty the fields they customized and try again; i.e., emptying the templates they customized should automatically revert those templates back to their default value, which would allow the upgrade to proceed then when they try it again. |
Referencing these lines of code that already handle the logic behind detecting changes in the template files. https://github.com/websharks/comment-mail-pro/blob/000000-dev/src/includes/classes/Plugin.php#L1084-L1095 That's why I say, if any of these options keys are not empty, they have made changes. |
Or any of these options keys. Sorry, I left these out before: https://github.com/websharks/comment-mail-pro/blob/000000-dev/src/includes/classes/Plugin.php#L714-L726 |
In other words, anything that has |
@jaswsinc I was just thinking about this problem some more (and about how I'm not looking forward to doing an interim release with the old codebase and hoping that everybody upgrades to that to see the Dashboard message before we can do another release with the new codebase) and I had an idea for another way we might be able to handle this: What if we just detect, as part of the new release, when someone has customized their Advanced Templates, and then simply not load those incompatible templates. Instead, we show a Dashboard notice letting the site owner know that until they update their templates the original default templates will be used. That seems like a safer plan than to hope that people update to the interim release and see the Dashboard notice. (If they don't update to the interim release and instead wait until the following release with the new codebase, then that update would produce fatal errors.) |
That sounds great. The only question I have is how will we allow them to see/revise their old templates exactly? I guess that's not impossible, but I'm curious to know what you feel would work best for that part; i.e., when they get a notice about having outdated templates, how do they access those old templates? |
@jaswsinc writes...
We could backup the contents of the customized Advanced Templates to new option keys in the database. Any templates they modified will then simply revert to their defaults in the plugin, so that the core functionality of the plugin doesn't break. We then show a Dashboard message like this:
To access the contents of the old customized templates, we simply check for the presence of those backed up option keys and have a new link that appears somewhere on each Advanced Template panel that will load the contents of the backed up template (which is stored in the new backed up option key). The KB Article will explain (with a screenshot) how to access the old template and describe how (and why) the Advanced Templates changed in an incompatible way. |
@jaswsinc Any thoughts on the above? ↑ |
Cool idea. Hmm, those AC template files are written in PHP. Maybe it would even be OK to just append the old template to the new one and then comment all of that out; e.g., |
Hmm, so that would basically look like this?
Do I have that right? I guess my only concern with going this route is that we're backing up their customizations by modifying template files inside the plugin itself, which means a future upgrade will simply overwrite those default templates with a copy that doesn't include their customizations. Or if something goes wrong (e.g., maybe they had some bad code in their template that somehow broke our attempt to append it to the bottom of the default template PHP file as a comment) and they need to replace the plugin files altogether, they'll have no way of recovering the backup of their custom advanced templates (since that backup would've been destroyed by deleting and reinstalling Comment Mail Pro). That was why backing things up to the database seemed safer... if they somehow delete the Comment Mail Pro database options, well, then all of their Comment Mail configuration would be lost and it would be expected at the Advanced Template customizations would be lost too. |
@jaswsinc Ping. ↑ |
Yes. Sounds good.
Now that you say that it makes me think of that a bonus with this technique, because if we go that route we are also forcing the updated template file to be different from the default template value; i.e., it will contain
Hmm. Well, if they have bad code in an advanced template file the site was already broken before the upgrade, and the upgrade would be more likely to fix the issue than it would be to break something further. So long as we wrap the old template in
I'm not against that. If that seems easier to you then I'm fine with that :-) |
@jaswsinc writes...
The problem with that is you'd have to first strip out all of the multiline comments from the existing [modified by the user] template, and the user may have added their own multiline comments to help remind themselves of their own changes, so we wouldn't want to strip those out when making a backup of their modified template. Why would you have to strip out multiline comments? Because PHP doesn't support nested multiline comments. See the PHP Manual:
|
Ah, good thinking! I totally forgot about that. Maybe we could instead do something like: <?php
$original_template = '....';
$new_template .= preg_replace('/^/um', '#', $original_template); |
We'd also need to deal with the |
Maybe just ignore any lines that start with <?php
# namespace WebSharks\CommentMail\Pro;
#
# /*
# * @var Plugin $plugin Plugin class.
# * @var Template $template Template class.
# ...
# * @note In addition to plugin-specific variables & functionality,
# * you may also use any WordPress functions that you like.
# */
?>
<?php // Sets document <title> tag via `%%title%%` replacement code in header.
# echo str_replace('%%title%%', __('Confirmation Request', SLUG_TD), $email_header); ?> |
↑ This is why it seemed easier to me to just copy any |
Though I do agree that it will be a lot easier from a support-perspective to be able to say "your customized template was copied to the bottom of the reset template; just go there to see your old version and you can then compare that to the reset version". So, I'm all for solving the problem of copying the old template to the bottom of the reset template, so long as we can do so in a reliable way that doesn't break things (which is what we're trying to avoid in the first place). Also, it's important to keep in mind that the user may have modified their template in any number of ways—we have no idea what it looks like now. |
Yikes. Yep, I see what you mean. Well, here is another idea. $original_template = preg_replace_callback('/\/\*(.*?)\*\//u', function($m) {
return preg_replace('/^/um', '# ', $m[1]);
}, $original_template);
$new_template .= "\n\n";
$new_template .= '/*';
$new_template .= $original_template."\n";
$new_template .= '*/'; In other words...
|
Hmm... some of the templates contain CSS/JS also, so it's not just PHP that we are dealing with. Yeah, that's getting a bit hairy now. Not sure I still like that idea either. |
Hmm, I hadn't thought of that either. Yeah, I'd say we should just go with my original idea of backing up the existing Then we just need to give the user access to that backed up data. |
Agree. Maybe there could be a toggle button in the template editor that would allow for the old version to be appended (like we have been discussing) as best we can. That way they will gain access to both the old and new together in the same file, but only for the purpose of reclaiming the old data whenever they explicitly ask for it; i.e., when editing templates. So we could try working from this idea and improve it. If we can get that somewhere close to 99% accurate, then it could be used in that toggle functionality, but not as a default VS upgrade. |
I suppose to the best way to present the backed up data from the user's perspective would be to have a link/button that expands a second code box underneath the one that contains the (now reset) template. That way they can easily compare the two, with syntax highlighting. Then another button/link that allows them to delete the backed up version, getting rid of that backed up option key. |
Yeah, that would be ideal. The downside is that it opens the door for new bugs in the UI, it will take more time to implement it, and it's all for the purpose of providing back compat. to only just a few people. |
If we do append the old template to the new in some sort of toggled mode that allows a site owner to reclaim the old data, we could also append a marker just before the old data. That way the old data can be excluded entirely whenever they save any changes from that page where templates are being edited. <?php
blah blah blah in the new template...
/* --- Original Template --- */
/*
Old template code.
*/ Whenever the templates are saved, we strip the marker and everything after it before saving. |
Hmm, I hadn't thought of that. I forgot this was Pro-only, which means right now at most we're affecting ~40 people with this, and even then not all 40 of them are going to have necessarily customized the Advanced templates... |
I really like that idea. That way the old template is never really being mixed with the live template. |
Maybe that's a better idea then. Instead of commenting out the old template, we just append it and add a marker so that the old templates do contain a copy of the old, but whenever the template is read and used by Comment Mail, we don't actually use anything after the marker and of course the marker itself would also be excluded too. If we went that route, there wouldn't need to be a toggle mode or anything, the VS upgrade routine would simply append the old to the new (if it had been customized), along with a marker. The template system can then be updated to look for that marker and automatically exclude it whenever the template is read and parsed. See: https://github.com/websharks/comment-mail-pro/blob/000000-dev/src/includes/classes/Template.php#L325 |
That would also mean that we don't even need to worry about commenting out the old code... all we need is a marker that indicates what comes next is just a copy of their old modified template. /* --------------------------- Original Template ---------------------------
* Comment Mail v16xxxx included changes that were not
* backwards compatible with your customized Advanced Template.
* To prevent problems with the upgrade to v16xxxx, we reset
* the Advanced Templates to their (new) default and backed up
* your customized template, a copy of which is included below.
* You can reference your original template below to reapply
* those changes to the new default template above.
* When you are ready to discard this backup, simply delete
* this comment, and everything below it, and save the template.
* If you leave this comment here, your backup will be retained.
*/
<?php
namespace WebSharks\CommentMail\Pro;
/*
* @var Plugin $plugin Plugin class.
* @var Template $template Template class.
*
* Other variables made available in this template file:
*
* @var \stdClass $sub Subscription object |
Great minds think alike :-) 😸 |
Actually, it would need to be implement here instead. |
That would work, but that does seem more complicated than simply backing up the option keys, injecting the backed up content into the textarea and then stripping it out whenever the templates are saved. Any reason you like your idea better? |
I like both of those ideas. However, yes, I do like my idea slightly better. The reason is that I see it taking fewer modifications to the code to bring it all together. For instance, no changes in the menu page class file would be needed so long as we don't introduce any new option keys that contain backup data. Here's a quick look at how I'd try to implement my idea:
With your idea, it provides a better separation, but then we would need to add some more steps.
|
Next 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](#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).
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 (#238). |
The Comment Mail codebase is being completely revamped to bring it up-to-date with PSR 2/4 standards (see #150). These changes will affect any Advanced PHP templates that have been customized in any version up to and including v160213.
If Comment Mail Advanced PHP templates were modified, the next release will produce fatal errors unless we can come up with a way to handle this gracefully.
What we need to do for the next release is to build a version upgrade routine that will detect and handle a scenario where a site owner has customized the Advanced PHP templates and then prevent a fatal error from being thrown, along with a Dashboard notice that links to a KB article explaining how to migrate Advanced PHP templates to the new codebase.
Referencing original discussion that led to this GitHub issue:
@jaswsinc writes in #150 (comment)...
@raamdev writes...
@jaswsinc writes...
@raamdev writes...
The text was updated successfully, but these errors were encountered: