to easily "Add a note to the backup" ;-)

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

PhilY created an issue. See original summary.

PhilY’s picture

Issue summary: View changes
couturier’s picture

Version: 8.x-4.0-alpha1 » 8.x-4.x-dev
Priority: Normal » Minor

There are still some major issues that need to be addressed with this module to make it functional for all users, and work is going pretty slowly. A new maintainer has been requested for months to help with the work, but no one so far with the skills to do so has come forward. So, I'm setting this feature request to a minor priority since there are so many other tasks that are huge as far as making this module viable in Drupal 8. If users are really interested in getting this feature for Drupal 8, it would be nice to see more followers and comments of support in this thread. Also, is someone able to adapt code from Drupal 7 to Drupal 8 and provide a patch for dev?

Dinu R.’s picture

Status: Active » Needs review
FileSize
1.06 KB

Patch attached.

PhilY’s picture

Thanks a lot Dinu for your patch but it seems to have no effect either on 4.0-alpha2 or 2017-May-09 dev releases on my Drupal 8.3.1 website.
In case my issue isn't clear I've attached Drupal 7 and Drupal 8 screen captures to help see what's missing in D8.

couturier’s picture

Status: Needs review » Needs work

We have a new maintainer now, Alex Andrascu and his team at Intellix, who are working toward making D8 a stable release. If you're still interested in this feature, it would be nice to have more work done on a usable patch. We are anticipating a new D8 release soon, so you might watch for that and then work on the patch from there. Also, you might want to mention this feature in the discussion at the D8 Backup and Migrate Roadmap.

MrPaulDriver’s picture

+1 for notes.

I used this feature all the time with D7

drupal a11y’s picture

+1 for notes - really miss this feature at the moment.

ikit-claw’s picture

So you want the same thing in D8 just a log note?

MrPaulDriver’s picture

Exactly like D7, whatever that was.

jaesperanza’s picture

+10 to bringing back the add note to the backup :)

Jon Pollard’s picture

Such a handy feature - I miss it!!

bmango’s picture

Yes, this would be very handy! Especially when testing modules out to note at what stage the backup was taken.

DamienMcKenna’s picture

Version: 8.x-4.x-dev » 5.0.x-dev
Component: Code » User interface
Anybody’s picture

+1 for this!

paulocs’s picture

Status: Needs work » Needs review
FileSize
5.64 KB

New patch arriving!

I added the description above the backup the title.
Btw I had to change the BackupMigrateInterface interface otherwise I would not be able to get the description as the backup method receives the $source_id and $destination_id only.

Status: Needs review » Needs work

The last submitted patch, 16: 2846650-16.patch, failed testing. View results

paulocs’s picture

Assigned: Unassigned » paulocs
paulocs’s picture

Assigned: paulocs » Unassigned
Status: Needs work » Needs review
FileSize
3.08 KB
2.66 KB

Here is a much simpler patch that there is no need to change the interface.

Anybody’s picture

Nice work @paulocs and indeed quite simple with huge benefit :)

Only thing I'm unsure about is the need to escape these variables perhaps:

+      $col['description'] = [
+        '#markup' => '<div title="' . $backup->getFullName() . '" class="backup-migrate-description">' . $backup->getFullName() . '</div>',
+      ];
+
+      if (!empty($backup->getMeta('description'))) {
+        $col['description']['#markup'] .= ' <div title="' . $backup->getMeta('description') . '" class="backup-migrate-description">' . $backup->getMeta('description') . '</div>';
+      }

Afterwards they run through the renderer, I guess it's OK, but someone else should have a look for #markup security reasons to be 100% sure.

hmendes’s picture

Hello @Anybody,
I've never done this before, how should this part of code look like?

Anybody’s picture

@hmendes: I'm also unsure about this, as I wrote, so perhaps one of the others in this issue can help?

If it becomes RTBC'd, could be a good candidate for #3223059: Plan for Backup and Migrate 5.1.0?

Anybody’s picture

Priority: Minor » Normal

@paulocs & @hmendes, here we go:
https://drupal.stackexchange.com/questions/184963/pass-raw-html-to-markup

I think inline_template would be nice and could also clean things up here with a {{description}} variable :)

aldairsoares’s picture

I'm going to review this one!

aldairsoares’s picture

Assigned: Unassigned » aldairsoares
Anybody’s picture

Status: Needs review » Needs work

Sorry should be "Needs work" as of #23

Grevil made their first commit to this issue’s fork.

Grevil’s picture

Status: Needs work » Needs review

I created an issue fork and applied the patch from #19.

I don't think, we need any additional escaping for the markup, since it already gets filtered through Xss::filterAdmin. See Render API overview for further information.

EDIT:

Note that the value is passed through \Drupal\Component\Utility\Xss::filterAdmin(), which strips known XSS vectors while allowing a permissive list of HTML tags that are not XSS vectors. (For example, [script] and [style] are not allowed.)

aldairsoares’s picture

I'm going to review it.

aldairsoares’s picture

Assigned: aldairsoares » Unassigned
Status: Needs review » Reviewed & tested by the community
FileSize
367.39 KB

After applying the last patch, I could use the functionality of add a note at quick backup.

I could not find any phpcs errors and the code looks good to me.

I attached a GIF showing the note added to quick backup.

I'm moving it to RTBC :D

Anybody’s picture

Thanks @Grevil for that additional information in #29! Great! :)

I agree with RTBC and would like to let @DamienMcKenna decide, if we can commit it this way or better put it into an inline_template. For me both is fine as we now know it's secure. :)

Thank you all! Would be cool to have this in the next release, as it's really helpful.

bruno.bicudo’s picture

+1 to RTBC

inline_template would indeed be more restrictive (as an additional escaping for markup). It could be better (maybe), but would it be necessary for this issue's scope?

I think using #markup here is enough and if needed it can be improved in the future :)

  • DamienMcKenna committed a831057 on 5.1.x
    Issue #2846650 by paulocs, Grevil, Dinu Rodnitchi, PhilY, aldairsoares,...
DamienMcKenna’s picture

Status: Reviewed & tested by the community » Fixed
Parent issue: » #3223059: Plan for Backup and Migrate 5.1.0

Committed. Thank you all!

DamienMcKenna’s picture

Version: 5.0.x-dev » 5.1.x-dev
Anybody’s picture

Thank YOU Damien that's great!! :) Small but very helpful.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.