Skip to content

Conversation

johnbillion
Copy link

See the individual commit messages for explanations of the changes.

@codecov-io
Copy link

codecov-io commented Aug 4, 2016

Codecov Report

Merging #1088 into master will decrease coverage by 6.24%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1088      +/-   ##
==========================================
- Coverage   32.05%   25.81%   -6.25%     
==========================================
  Files          31       31              
  Lines        2561     3653    +1092     
==========================================
+ Hits          821      943     +122     
- Misses       1740     2710     +970
Impacted Files Coverage Δ
classes/class-email-service.php 0% <0%> (ø) ⬆️
.../backup/class-backup-engine-database-mysqldump.php 78.78% <0%> (-7.18%) ⬇️
...es/backup/class-backup-engine-file-zip-archive.php 79.16% <0%> (-4.17%) ⬇️
...backup/class-backup-engine-database-imysqldump.php 82.6% <0%> (-3.11%) ⬇️
classes/backup/class-backup-engine-file-zip.php 78.12% <0%> (-2.65%) ⬇️
classes/backup/class-backup.php 71.17% <0%> (-2.4%) ⬇️
classes/backup/class-backup-engine-database.php 87.27% <0%> (-2.32%) ⬇️
classes/class-scheduled-backup.php 46.34% <0%> (-2.25%) ⬇️
functions/core.php 20.19% <0%> (-1.85%) ⬇️
classes/class-path.php 86.36% <0%> (-1.84%) ⬇️
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update beb970c...d1a87b5. Read the comment docs.

__( 'You can define any of the following constants in your %1$s file to control advanced settings. <a href="%2$s">The Codex can help</a>. Defined constants will be highlighted.', 'backupwordpress' ),
'<code>wp-config.php</code>',
'https://codex.wordpress.org/Editing_wp-config.php'
); ?></p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johnbillion is there a reason why these aren't escaped?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Escaping translations is something that I personally recommend strongly, but isn't strictly necessary. I think BWP should escape all of its translations, but that's something that would be better one all at once, and separately from these other enhancements.

esc_html__( 'If the errors above look like Martian, forward this email to %3$s and we\'ll take a look', 'backupwordpress' ) . "\n\n" .
home_url(),
$error_message,
'[email protected]'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johnbillion I wanted to make this one string, but then how would I deal with \n\n? Just add them like that in the string? There are few vars like this in this file. Are they ok to be merged into one string?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ok as separate strings with "\n\n" concatenating them.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

home_url() needs to be escaped. Is $error_message safe? This whole string should be wrapped in esc_html() instead of the individual esc_html__() usage.

'<p>' . sprintf(
__( 'BackUpWordPress Pro supports Dropbox, Google Drive, Amazon S3, Rackspace, Azure, DreamObjects and FTP/SFTP. <a href="%s" target="_blank">Check it out at bwp.hmn.md</a>', 'backupwordpress' ),
'https://bwp.hmn.md/?utm_source=wordpress-org&utm_medium=plugin-page&utm_campaign=freeplugin'
) . '</p>' .
Copy link
Contributor

@dashaluna dashaluna Aug 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pdewouters I think you've mentioned that these files have to have a specific format otherwise they won't be processed. Will it cope if we use sprintf() and esc_html__() ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep

@willmot
Copy link
Contributor

willmot commented Nov 4, 2016

@Dasha are you still working on this? Any idea when it might be done?

@mikeselander
Copy link

Since this overlays #1090 so closely, I think we could probably work on this one after that gets merged. It is quite stale but could be re-picked up.

@dashaluna
Copy link
Contributor

Wow, not quite sure why it wasn't merged before as it was in pretty good state. Not sure how to better approach this now as things still need fixing.

@dashaluna dashaluna assigned dashaluna and unassigned pdewouters Aug 30, 2017
@dashaluna
Copy link
Contributor

I'm picking this up.

esc_url( home_url() ),
$error_message,
'[email protected]'
);
Copy link
Contributor

@dashaluna dashaluna Aug 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johnbillion How does this look? Addressing your feedback that the whole message should be a one string. Ref 93c7a40#r75849252

@dashaluna
Copy link
Contributor

Right, let's do a final push on this one :) Can someone review please?
/cc @willmot

Copy link

@jazzsequence jazzsequence left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, although the tabbing in a couple places looked weird.

esc_html__(
'BackUpWordPress was unable to backup your site %1$s.' . "\n\n" .
'Here are the errors that we\'ve encountered: %2$s' . "\n\n" .
'If the errors above look like Martian, forward this email to %3$s and we\'ll take a look.' . "\n\n", 'backupwordpress' ),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tabbing here is weird

'BackUpWordPress has completed a backup of your site %1$s'. "\n\n" .
'The backup file should be attached to this email.' . "\n\n" .
'You can download the backup file by clicking the link below:' . "\n\n" . '%2$s' . "\n\n" .
'Kind Regards,\nThe Happy BackUpWordPress Backup Emailing Robot', 'backupwordpress' ),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tabbing here is also weird.

'BackUpWordPress has completed a backup of your site %1$s' . "\n\n" .
'Unfortunately, the backup file was too large to attach to this email.' . "\n\n" .
'You can download the backup file by clicking the link below:' . "\n\n" . '%2$s' . "\n\n" .
'Kind Regards,\nThe Happy BackUpWordPress Backup Emailing Robot', 'backupwordpress' ),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...and here.

@dashaluna
Copy link
Contributor

dashaluna commented Sep 1, 2017

@willmot I'm not sure what the process of preparing a new version is and when to merge PRs. But this is ready to go in 👍

@willmot
Copy link
Contributor

willmot commented Sep 5, 2017

@pdewouters any availability to handle review/merge and release?

@dashaluna
Copy link
Contributor

@pdewouters pinging about this one :)

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants