ryancramerdesign / ProcessDatabaseBackups

Create and/or restore database backups.
7 stars 1 forks source link

Errors on 2.7 dev #2

Open outflux3 opened 8 years ago

outflux3 commented 8 years ago

When i go to download a backup, i'm getting a redscreen...

Error: Exception: Unknown Selector operator: '' -- was your selector value properly escaped? (in /home/.../.../wire/core/Selectors.php line 283)

horst-n commented 8 years ago

I guess the Error comes from another (third party) admin-module that try to fetch the id of the currently edited page in the admin. Error comes from passing it like so: $pages->get($input->get->id) But ProcessDatabaseBackup is responsible for confusing the other modules a bit by using id as param name with a string and not a simple integer. :)

outflux3 commented 8 years ago

ah - ok i will see if i can narrow down which module is doing this and report back... Edit: well i can't seem to locate the issue, i'll have to clone the site to a local and selectively disable the modules to figure it out... at least it can still generate the backups and i can download them via ftp

horst-n commented 8 years ago

I have done a patch that solves this issue. You can find it here: https://github.com/horst-n/ProcessDatabaseBackups

outflux3 commented 8 years ago

hey - great - yeah this totally fixes it, just tested - thanks!

outflux3 commented 8 years ago

i'm having trouble now downloading the backups - i'm on 2.7.3 dev

Fatal error: Class 'wireTempDir' not found in //site/modules/ProcessDatabaseBackups/ProcessDatabaseBackups.module on line 335

horst-n commented 8 years ago

Uhm? Class wireTempDir is a core part and should be there in PW 2.7+ I will commit an update with a check for the class existence.

horst-n commented 8 years ago

Committed it!

outflux3 commented 8 years ago

wireTempDir is certainly there in the core; not sure why the module is not finding it...

your latest version does fix it though!

jlahijani commented 6 years ago

Any news on fixing this permanently? I have this issue on 3.0.84.

jlahijani commented 6 years ago

@outflux3 I tried disabling every module and that didn't work either.

horst-n commented 6 years ago

The relevant part, that I have committed already 1 1/2 years ago are this lines: https://github.com/ryancramerdesign/ProcessDatabaseBackups/blob/921706ff31be56e2fad5ab53d2d08621468cd66e/ProcessDatabaseBackups.module#L153 and https://github.com/ryancramerdesign/ProcessDatabaseBackups/blob/921706ff31be56e2fad5ab53d2d08621468cd66e/ProcessDatabaseBackups.module#L88-L91

@ryancramerdesign: Using id with the $input var is the culprit. It confuses other modules or hooks in your code that are executed on init, and try to get a $page->id for something not related with the DB-Backup module! This other modules or hooks code has not to be executed when DB-backup module is running, but get tricked by the presence of $input->get->id and kick in by this failure!

The database module should not use the keyname "id" within the GET formsubmissions. Many other codes in PW only uses $input->get->id to fetch the id of the currently edited page. The database module have to use another key name then "id". Simply change it to something other like "myid" would solve those issues!

ryancramerdesign commented 6 years ago

I understand that the use of "id" might be confusing another module (which one?), but that is not a problem with the backups module, that is a problem with whatever module is making that assumption. After all, numerous other modules use "id" in GET variables to refer to other asset types, including Core modules like ProcessTemplate and ProcessField, and 3rd party modules like this one and FormBuilder, as examples.

Another module pulling $_GET['id'] and assuming it is for a page is a potentially serious security problem. It's not something we should try to hide. It's something that should be exposed, so that it can be fixed by whatever module is making the assumption. In ProcessWire "id" is used by all asset types, not just Page. The name "id" has nothing to do with pages, unless you are actually editing a Page. Here's how that can be determined:

if($this->wire('process') instanceof WirePageEditor) {
  $page = $this->wire('process')->getPage(); 
  if($page->id) {
    // page is being edited
  }
}

In some rare cases a module might need to initialize before the current $process has, in which case you might need to pull from the $_GET['id']. Here's how you can make sure that the ID would be related to a Page:

if(in_array('WirePageEditor', wireClassImplements((string) $this->wire('page')->process))) {
  $id = (int) $this->wire('input')->get('id');
  if($id) {
    $page = $this->wire('pages')->get($id); 
    if($page->editable()) {
      // page is being edited
    }
  }
}

There are also other safe ways to perform this check that might apply in other cases.

horst-n commented 6 years ago

Ryan, all you say is right. I just wanted to help and recap all my findings from 1 1/2 years ago. But I don't have the time to view and search for those old posts here and in the forums. I remember that I run into this issue when I first had installed dbbackup and AOS in its first days. After found out that AOS assumed the id should be a page id, tpr changed it directly. BUT you Ryan and the very first PW heros cluttered the forums with countless code examples that propagate exactly the wrong way of reading in the id of a currently used or edited page. So thinking of newcomers who want to try out things and borrow code peaces from veterans of the forums wirh more than 4k reputation and than run into such an issue, I hardly can imagine they will understand whats going on or are able to fix it.

When I run into that issue, I needed more then 3-4 attempts do debug and finally locate the issue. Tooks me 7-8 hours!

So, it is up to you Ryan, change the keyname or leave it as is. Don't cover the wrong assumptions of module authors or authors of code snippets defined in init.php, (and let the 50+ wrong code examples covered in the forums), or make it a bit easier for a lot of folks that only follow(ed) your code examples from the earlier days.

I'm not able to decide which route is better. Maybe only a political decision is doable for this dilemma. And I'm not good in politics. :)

This is not offending in any way, I only wanted to help for a better understanding of a bit bigger picture of that issue. Your usage of id in the dbbackup module is correct, but it tricks a lot folks who only followed your code advise from forum snippets.