Skip to main content

This site requires you to update your browser. Your browsing experience maybe affected by not having the most up to date version.

We've moved the forum!

Please use forum.silverstripe.org for any new questions (announcement).
The forum archive will stick around, but will be read only.

You can also use our Slack channel or StackOverflow to ask for help.
Check out our community overview for more options to contribute.

General Questions /

General questions about getting started with SilverStripe that don't fit in any of the categories above.

Moderators: martimiz, Sean, Ed, biapar, Willr, Ingo, swaiba

Problem with a form and handling data


Go to End


10 Posts   2908 Views

Avatar
J2-Paul

Community Member, 51 Posts

6 June 2010 at 1:44am

I'm new to PHP and therefore SS too. So forgive if this is entry level stuff.

I have worked through the tutorials and now I am trying my own stuff, setting myself some tasks.

I have defined a page that I use to search for students and then bring up and edit form. The page is working fine. But I am convinced the code could be better.

See my comment in the "doUpdateStudent" method. This explains the bit I am trying to improve.

Help, comments, much appreciated.

/ EDIT STUDENT RECORDS.

// Student Edit form
function StudentEdit() {
Us

///Get studentdata for specified ID in url.
$student = DataObject::get_by_id('Student',$_GET['id']);

$fields = new FieldSet(
new HiddenField("ID","","$student->ID"),
new TextField("FirstName","First Name","$student->FirstName"),
new TextField("Lastname","Last Name","$student->Lastname"),
new TextField("Nationality","Nationality","$student->Nationality")
);
$actions = new FieldSet(
new FormAction('doUpdateStudent','Save'),
new FormAction('StudentEditCancel','Back to list')
);

$validator = new RequiredFields('FirstName','Lastname','Nationality');

return new Form($this,'StudentEdit',$fields,$actions,$validator);

}
// Returns the previous page.
function StudentEditCancel(){
Director::redirect('.');
}

// Update student record

function doUpdateStudent($data,$form) {

// get record.... again!!!! <<< this is the bit that I think I should not have to do
$student = DataObject::get_by_id('Student',$data['ID']);

// Update object values with current form values
$student->FirstName=$_POST['FirstName'];
$student->Lastname=$_POST['Lastname'];
$student->Nationality=$_POST['Nationality'];
// Write out record to table.
$student->write();

Director::redirectback();

}

}

Avatar
dhensby

Community Member, 253 Posts

6 June 2010 at 2:49am

Edited: 06/06/2010 2:50am

JHi J2-Paul,

I hope you are enjoying SS as you get to grips with it.

Firstly, i'd say what your doing is fine. You have to pull the student record out again because the form and the form handler will be called on different page loads, so not ready to access from the previous function's call.

a couple comments though:
Instead of using the $_GET global, use the $data array that has been passed to you. You should also check that the student infact exists before trying to write to it. In fact, you can simply do:

 function doUpdateStudent($data,$form) { 

      // get record.... again!!!! <<< this is the bit that I think I should not have to do 
      $student = DataObject::get_by_id('Student',$data['ID']); 
      if ($student) { 
     $form->saveInto($student);

      // Write out record to table. 
      $student->write();       
      }
       
      Director::redirectback(); 
       
    
   }

One last comment is on security, make sure when you do DataObject::get() that your 'where' clause (or filter) has sanatised input. eg: If you are getting data submitted by a user, make sure the data is valid and in the format you expect. Here is how i would do your student lookup:

if (isset($_GET['ID']) && is_integer($_GET['ID']) && $_GET['ID'] > 0) {
$student = DataObject::get_by_id('Student',(int)$_GET['ID']);
}
else {
return $this->httpError(404,'Sorry, there was a problem finding that student');
}

So i am checking that the ID has been set for a start, then i am checking it is an integer and that it is above 0. I then dictage the the data type is passed as an integer to the get_by_id function. This will stop your users seeing errors. If the verification of the $_GET var fails, then a 404 is thrown with a custom error message.

DataObject::get_by_id is an exception that checks the passed filter is checked (SS checks it is numeric, but that doesnt mean we shouldnt be in the habbit of doing it too).

This would be very dangerous:

DataObject::get('Student',"Nationality = '".$_GET['nationality'] . "'");

and should be:
DataObject::get('Student',"Nationality = '".Convert::raw2sql($_GET['nationality']) . "'");

Reading this would be useful:
http://doc.silverstripe.org/secure-development

I hope that is all useful,

Enjoy!

Avatar
J2-Paul

Community Member, 51 Posts

6 June 2010 at 8:00am

Hi Pigeon and thanks for the prompt response. Much appreciated.

Yep. Really enjoying SS. Early days yet, but the signs are good!

Really like the code "$form->saveInto($student)"; much more elegant. I think it is little faster too, which is always a good thing!

If I understand this correctly the saveInto method updates the properties of the $student object with the corresponding properties of the $form object. I did a print_r($form) to see if I could follow this, but there is a bunch of data in the object, so for now I am just going to have to trust in this one until I get a better understanding of objects and and properties.

However, I have a problem with the suggested security code. Not matter what I have tried I can not make it work.

Take a look at my code below. Not this same as you suggested, but along the same lines.

If I comment out the 404 line. Then all is fine it works as before.

However, we the 404 line in place. Both the Save button and cancel button throw 404s. There is a clue.

1. For the "back to list" button it redirects to "/student-search/StudentEdit" (without the 404 line it comes back "/student-search"
2. For the save button it redirects to "/student-search/StudentEdit" too. (without the 404 line it comes back /student-search/ShowStudent?id=x

I get the same behaviour when I used your code verbatim.

Any ideas?

// Student Edit form called by template.
function StudentEdit() {

// Get studentdata for specified "?id=" in url.

$student = DataObject::get_by_id('Student',$_GET['id']);

// give error if record not found
if (!$student) {
return $this->httpError(404,'Sorry, there was a problem finding that student');
}

/// put some not found code here

$fields = new FieldSet(
new HiddenField("ID","","$student->ID"),
new TextField("FirstName","First Name","$student->FirstName"),
new TextField("Lastname","Last Name","$student->Lastname"),
new TextField("Nationality","Nationality","$student->Nationality")
);
$actions = new FieldSet(
new FormAction('doUpdateStudent','Save'),
new FormAction('StudentEditCancel','Back to list')
);

$validator = new RequiredFields('FirstName','Lastname','Nationality');

return new Form($this,'StudentEdit',$fields,$actions,$validator);

}
// Returns the previous page.
function StudentEditCancel(){
Director::redirect('.');
}

// Update student record

function doUpdateStudent($data,$form) {

//print_r($data);

// get object again. Necessary because of page reload. (formum help Pigeon)
$student = DataObject::get_by_id('Student',$data['ID']);

// Update object values with current form values
if ($student) {
$form->saveInto($student);
$student->write();
}

Director::redirectback();

}

Avatar
dhensby

Community Member, 253 Posts

6 June 2010 at 10:26am

I'm glad you are enjoying SS and you like the saveInto() method, i think it does work as you assume, but you can always look in the Form class to check!

Ah, the issue on the form actions will be that $_GET['id'] is not set, so there won't ever be a student object. Is your code not trowing undefined variable errors?

try:

if (isset($_GET['id']) && is_integer($_GET['id']) && $_GET['id'] > 0) {
 $student = DataObject::get_by_id('Student',$_GET['id']);
}

if (!isset($student) || !$student) {
$this->httpError(404,'blah');
}

On the print_r($form) note: to get an idea of what an object contains use the Debug class:

Debug::show($form);

or for a more raw idea of the object, you can always use var_dump($form)

Finally, you are using Director::redirectBack(), and Director::reditect() - which is fine - but they are also inclued as part of the controller object so you can do this instead:

$this->redirectBack();

or
$this->redirect('/');

I know the docs say 'Director::redirectBack()' but i think they are a little out-dated and the convention is moving more towards $this->redirectBack() in the controller.

Avatar
dhensby

Community Member, 253 Posts

6 June 2010 at 10:37am

Oh, and one other thing, when creating your form, you don't have to dictate all the default values of the fields like you have, you can do this instead:

  $fields = new FieldSet( 
         new HiddenField("ID",null), 
         new TextField("FirstName","First Name"), 
         new TextField("Lastname","Last Name"), 
         new TextField("Nationality","Nationality") 
         ); 
      $actions = new FieldSet( 
         new FormAction('doUpdateStudent','Save'), 
         new FormAction('StudentEditCancel','Back to list') 
         ); 
          
      $validator = new RequiredFields('FirstName','Lastname','Nationality'); 
       
      $form = new Form($this,'StudentEdit',$fields,$actions,$validator);

     $form->loadDataFrom($student);

     return $form;

When you are parsing variables to methods like:

new TextField('Name','First Name',$var);

It is quicker to do it without the quote marks around the $var (quicker in terms of typing and PHP execution) because there is no need for PHP to figure out there is no string in the quotes.

Personally i concatenate all my strings and vars like:

return 'The student\'s name is: ' . $student->Name . ' and they are ' . $this->Nationality . '!';

Avatar
J2-Paul

Community Member, 51 Posts

6 June 2010 at 1:10pm

More good stuff particular the loadDataFrom() method. I need to read up on the methods in the form class clearly!

However, I have now seem to have gone backwards. After many attempts getting this working. I reverted back to my original code which I swear blind was working. But, of course, now it is not!

My problem seems to be with the "StudentEdit() method. This is called by a template that is called in the ShowStudent method.

When this form loads neither of the formactions are working and I can not figure out why.

Below is the full code for the page. If you could give me a steer here it would be greatly appreciated.

<?php

class StudentSearch extends Page {

static $db = array();
static $has_one = array(
);

}

class StudentSearch_Controller extends Page_Controller {

// SEARCH FOR STUDENT RECORDS

// Search form
function StudentSearch() {
//Create Search box fields

// Set search string to
$searchstring=Session::get('SearchString');

//this is not working. not sure why!
$searchText = isset($searchsting) ? $searchstring : 'Enter Student Name';

$fields = new FieldSet(
new TextField("Student","Name:",$searchstring)
);
$actions = new FieldSet(
new FormAction('doStudentSearch','Search for Student'),
new FormAction('doGoGoogle','Go to Google')
);

$validator = new RequiredFields('Student');
return new Form($this,'StudentSearch',$fields,$actions,$validator);

}
// Run Search
function doStudentSearch($data,$form){

Session::set('SearchString', $data['Student']);
debug::show($data);
Director::redirectBack();

}

function doGoGoogle($data,$form){

Session::set('SearchString', $data['Student']);
// debug::show($data);
Director::redirect('http://www.google.co.uk');

}

// Get student search results for display on template
function StudentSearchResults(){

$searchstring=Session::get('SearchString');
$results = DataObject::get('Student',"`Lastname`='$searchstring'",'Lastname,FirstName');

return $results;

}

// Load Edit form for student record
function ShowStudent(){
// This template calls the StudentEdit Method below.
return $this->renderWith(array('StudentSearch_Editform', 'Page'));
}

// EDIT STUDENT RECORDS.

function StudentEdit() {

///Get studentdata for specified ID in url.
$student = DataObject::get_by_id('Student',$_GET['id']);

$fields = new FieldSet(
new HiddenField("ID","","$student->ID"),
new TextField("FirstName","First Name","$student->FirstName"),
new TextField("Lastname","Last Name","$student->Lastname"),
new TextField("Nationality","Nationality","$student->Nationality")
);
$actions = new FieldSet(
new FormAction('doUpdateStudent','Save'),
new FormAction('StudentEditCancel','Back to list')
);

$validator = new RequiredFields('FirstName','Lastname','Nationality');

return new Form($this,'StudentEdit',$fields,$actions,$validator);

}

// Returns the previous page.
function StudentEditCancel($data,$form){

// debug::show($data);
Director::redirect('.');
}

// Update student record

function doUpdateStudent($data,$form) {

debug::show($date);
// get record.... again!!!! <<< this is the bit that I think I should not have to do
$student = DataObject::get_by_id('Student',$data['ID']);

// Update object values with current form values
$student->FirstName=$_POST['FirstName'];
$student->Lastname=$_POST['Lastname'];
$student->Nationality=$_POST['Nationality'];
// Write out record to table.
$student->write();

Director::redirectback();

}

}
?>

Avatar
dhensby

Community Member, 253 Posts

7 June 2010 at 12:48am

Ok, I'm not sure what errors you are getting, but i have noticed that there are typos and other issues in that code. things like 'redirectback()' should be 'redirectBack()' and debug::show($date); which should be Debug::show($data);

I went through your code very quickly and made some changes. I'm sure you will need to tweak it a little, but give it a go:

<?php 

class StudentSearch extends Page { 

	static $db = array(
		
	);
	
	static $has_one = array(
		
	); 

} 

class StudentSearch_Controller extends Page_Controller { 

// SEARCH FOR STUDENT RECORDS 

	// Search form 
	function StudentSearch() { 
		//Create Search box fields 
		
		// Set search string to 
		$searchstring=Session::get('SearchString'); 
		
		//this is not working. not sure why!
		# $searchstring will always be set (it is set above) so you probably want to check a value
		$searchText = $searchsting ? $searchstring : 'Enter Student Name'; 
		
		$fields = new FieldSet( 
			new TextField("Student","Name:",$searchstring) 
		); 
		$actions = new FieldSet( 
			new FormAction('doStudentSearch','Search for Student'), 
			new FormAction('doGoGoogle','Go to Google') 
		); 
		
		$validator = new RequiredFields('Student');
		
		return new Form($this,'StudentSearch',$fields,$actions,$validator); 
	}
	
	// Run Search 
	function doStudentSearch($data,$form){ 
		
		if (isset($data['Student'])) {
			Session::set('SearchString', $data['Student']);
		}
		
		#no need to debug right now
		#Debug::show($data); 
		$this->redirectBack();    
	
	} 
	
	function doGoGoogle($data,$form){ 
	
		if (isset($data['Student'])) {
			Session::set('SearchString', $data['Student']);
			$this->redirect('http://www.google.co.uk/search?q=' . $data['Student']);
		}
		else {
			$this->reditect('http://www.google.co.uk/');
		}
		//debug::show($data); 
	
	} 
	
	// Get student search results for display on template 
	function StudentSearchResults(){ 
	
		$searchstring=Session::get('SearchString');
		
		$results = DataObject::get('Student',"`Lastname`='".Convert::raw2sql($searchstring)."'",'Lastname,FirstName'); 
		
		return $results; 
	
	} 
	
	// Load Edit form for student record 
	function ShowStudent(){ 
		// This template calls the StudentEdit Method below. 
		return $this->renderWith(array('StudentSearch_Editform', 'Page')); 
	} 
	
	// EDIT STUDENT RECORDS. 
	
	function StudentEdit() { 
	
		///Get studentdata for specified ID in url. 
		$student = DataObject::get_by_id('Student',$_GET['id']); 
		
		$fields = new FieldSet( 
			new HiddenField("ID","","$student->ID"), 
			new TextField("FirstName","First Name","$student->FirstName"), 
			new TextField("Lastname","Last Name","$student->Lastname"), 
			new TextField("Nationality","Nationality","$student->Nationality") 
		); 
		$actions = new FieldSet( 
			new FormAction('doUpdateStudent','Save'), 
			new FormAction('StudentEditCancel','Back to list') 
		); 
		
		$validator = new RequiredFields('FirstName','Lastname','Nationality'); 
		
		return new Form($this,'StudentEdit',$fields,$actions,$validator); 
	
	} 
	
	// Returns the previous page. 
	function StudentEditCancel($data,$form){ 
		
		//      debug::show($data); 
		Director::redirect('.'); 
	} 
	
	// Update student record 
	
	function doUpdateStudent($data,$form) { 
	
		#debug::show($date); 
		// get record.... again!!!! <<< this is the bit that I think I should not have to do 
		$student = DataObject::get_by_id('Student',$data['ID']); 
		
		// Update object values with current form values 
		$student->FirstName=$_POST['FirstName']; 
		$student->Lastname=$_POST['Lastname']; 
		$student->Nationality=$_POST['Nationality']; 
		// Write out record to table. 
		$student->write(); 
		
		$this->redirectBack(); 
	
	} 




}

Avatar
J2-Paul

Community Member, 51 Posts

7 June 2010 at 2:01am

Daniel. I really appreciate your feedback. More good stuff which I have feed in to my code.

Since my last post I have tried another approach and I think the below gives a better insight in to my issue.

Here is some more detail and changes I have made. Hope you can follow OK. I appreciate that there are still some work to do on security. However, I need to get the thing working first!

I have a page that works fine that renders a list of students in a table on a page "/student-search/"

On the list of students I hyperlink each row thus: href="/student-search/edit?id=xxxxx" xxxx=ID number in dataobject "student"

I have defined a page class "StudentEdit" (below) and created a page called "edit" using this pageclass the purpose of the page is of course to edit the student records.

The page calls the subclass form studentform and renders fine and the displays URL is, for example /student-search/edit?id=123

My problem is that the form actions both change the url to "/student-search/edit/StudentEdit" which tells me they are calling the StudentEdit method and not the methods defined in the studentform subclass for the form actions.

I have tried moving the formaction methods back into the parent studentedit page controller. But this did not work.

What am I missing? Thanks again for your help!

Paul

------------------------

<?php

class StudentEdit extends Page {

static $db = array();
static $has_one = array(
);

}

class StudentEdit_Controller extends Page_Controller {

// SEARCH FOR STUDENT RECORDS

// EDIT STUDENT RECORDS.

function StudentEdit() {

if (isset($_GET['id'])) {

$student = DataObject::get_by_id('Student',$_GET['id']);

// Debug::show($student);

if ($student) {
$form= new StudentForm($this,'StudentEdit');
$form->loadDataFrom($student);
return $form;
}
}
else{
return $this->httpError(404,'Sorry, student does not exist');

}

}

}

?>

-------------------------------------------

<?php

class StudentForm extends Form {

function __construct($controller,$name) {

$fields = new FieldSet(
new HiddenField("ID","",""),
new TextField("FirstName","First Name",""),
new TextField("Lastname","Last Name",""),
new TextField("Nationality","Nationality","")
);
$actions = new FieldSet(
new FormAction('doUpdateStudent','Save'),
new FormAction('StudentEditCancel','Back to list')
);

$validator = new RequiredFields('FirstName','Lastname','Nationality');

parent::__construct($controller,$name,$fields,$actions);

}

// Returns the previous page.
function StudentEditCancel($data,$form){
// debug::show($data);
$this->redirect('.');
}

// Update student record

function doUpdateStudent($data,$form) {

Debug::show($data);
$student = DataObject::get_by_id('Student',$data['ID']);

if ($student) {
// update $student object wit form values
$form->saveInto($student);
// update record in student table
$student->write();
}

$this->redirectBack();

}

}

?>

Go to Top