Pass PHP callback to constructor

[NotionCommotion]

How do I pass a callback function to a constructor, and be able to use variables from both the parent class and new class?

 

The sample code below will hopefully make my question more clear.

 

Thank you

 

<?php
$controller=new controller();
$controller->doSomething();

class controller
{
    public $pdo;
    public $id=333;

    public function __construct()
    {
       $this->pdo= new PDO ('mysql:dbname=testdb;host=127.0.0.1', 'dbuser',  'dbpass');
    }

    public function doSomething()
    {
        $arr=array(
           'a'=>123,
           'b'=>321,
           'save'=>  function($col1,$col2) {
              $stmt=$this->pdo->prepare('UPDATE mytable SET col1=?, col2=? WHERE id=?');
              $stmt->execute($col1,$col2,$this->id);
           }
        );
        new myClass($arr);
    }
}


class myClass
{
    public function __construct($options)
    {
       // Do some stuff...
       $col1=111;
       // Assume $_POST['col2_value'] is equal to 222
       $options['save']($col1, $_POST['col2_value']);  //Execute 'UPDATE mytable SET col1=111, col2=222 WHERE id=333'
    }
}

Edited September 30, 2015 by NotionCommotion

[Jacques1]

What exactly is the problem with the code you already have? If you're running at least PHP 5.4, this should behave just like you want it to. In earlier versions you have to explicitly import the reference from $this into the closure's scope:

<?php

class A
{
    public $foo = 123;

    public function getClosure()
    {
        $definerInstance = $this;
        return function() use ($definerInstance)
        {
            var_dump($definerInstance->foo);
        };
    }
}

$a = new A();
$closure = $a->getClosure();
$closure();

Note the use keyword.

 

But what are you even trying to do? This approach looks a bit odd.

[scootstah]

That is a very bad approach. You should be using actual, structured classes and not arbitrary arrays with anonymous functions.

 

Also, for the record, constructors should only be concerned with setting up the class... and not actually doing anything.

[requinix]

I agree with scootstah: this is better served with an interface and an implementing class. Closures/callbacks/anonymous functions are more for simple tasks, like how Jacques's example where it just dumps out a value. The mere fact that the function is named "save" suggests it should conform to a specific signature (be that ($col1, $col2) or (array $cols)) and thus in a class.

 

Fun fact: PHP 7 supports anonymous classes, so one could do... well, it doesn't look particularly pretty so I won't post the example code for it. But if you're concerned about lots of one-off classes, this would be marginally better.

[NotionCommotion]

Hi Jacques1.  Long time no see.  Hope all is good.

 

Maybe no problems with the code.  At my day job now and wrote it without trying it.  I've never tried to do something like this, and didn't know I was close.

 

What am I trying to do?  I wrote a little jQuery plugin which allows the user to select an image file on their local PC and then select the area of the image which they want, and the file and desired area coordinates are sent to the server.  It also allows the user to delete the previously uploaded image and replace the target with a default image.  The JavaScript is similar to:

$('.target').imageCropper('/path/to/server/script.php?task=doSomething');

To accompany the plugin, I also wrote a PHP class which has four public methods, and the method to execute is based on $_GET or $_POST['method_to_execute'].

1. init().  The configuration of the plugin is all defined serverside and the plugin gets the configuration settings from the server when it is initialed.
2. upload().  Uploads the image and saves it in a temporary folder.
3. update().  Crops and resizes the image and moves it to a permanent folder.
4. delete().  Deletes the previously image which is stored in the permanent folder.

For the update() and delete() methods, I wish to allow some other custom functionality such as updating the database.

 

The PHP configuration would be something similar to the following:

    public function doSomething()
    {
        $arr=array(
           'path_to_save'=>'bla/bla/bla',
           'ratio'=>2,
           'default_image'=>'/bla/default.png',
           'allowed_extentions'=>array('png','jpg'),
           'other_stuff_to_override_default_settings'=>'bla',
           'save'=>  function($col1,$col2) {
              // call back to save filename in database
              $stmt=$this->pdo->prepare('UPDATE mytable SET col1=?, col2=? WHERE id=?');
              $stmt->execute($col1,$col2,$this->id);
           },
           'delete'=>function() {
              //As required
           }
        );

        new imageCropper($arr);  //Based on $_GET or $_POST['method_to_execute'], execute the appropriate method.
   }

Make sense?

Edited September 30, 2015 by NotionCommotion

[Jacques1]

Consider using the observer pattern instead of callbacks to add arbitrary actions on top of the standard update() and delete() action.

[NotionCommotion]

Hello Scootstah and Requinix,

 

I replied to Jacques1 give some context on what I am attempting to accomplish.  Does your advise change?

 

Thanks

[NotionCommotion]

Consider using the observer pattern instead of callbacks to add arbitrary actions on top of the standard update() and delete() action.

 

I never heard of observer pattern before and am reading up on it.  How do you envision it being used?  Thanks

[Jacques1]

Observers are, roughly speaking, “callback objects” which are notified by the basic class when a particular action happens (e. g. an image upload) and may then add their own functionalities.

 

In your case, the interface for an upload observer would look something like this:

interface ImageUploadObserver
{
    public function onUpdate(array $imageData);

    public function onDelete(array $imageData);
}

Now you can implement a specific observer which saves the image data in a database:

class ImageArchiver implements ImageUploadObserver
{
    public function onUpdate(array $imageData)
    {
        echo "Updating image $imageData[image_id] in database.<br>";
    }

    public function onDelete(array $imageData)
    {
        echo "Deleting image $imageData[image_id] from database.<br>";
    }
}

The image uploader class maintains a list of observer objects which are notified when an image is updated or deleted:

class ImageUploader
{
    protected $observers = [];

    public function registerObserver(ImageUploadObserver $observer)
    {
        $this->observers[] = $observer;
    }

    public function update(array $imageData)
    {
        echo "Updating image $imageData[image_id].<br>";

        // notify observers
        foreach ($this->observers as $observer)
        {
            $observer->onUpdate($imageData);
        }
    }

    public function delete(array $imageData)
    {
        echo "Deleting image $imageData[image_id].<br>";

        // notify observers
        foreach ($this->observers as $observer)
        {
            $observer->onDelete($imageData);
        }
    }
}

And a complete example:

$imageUploader = new ImageUploader();

$imageArchiver = new ImageArchiver();
$imageUploader->registerObserver($imageArchiver);

$imageUploader->update(['image_id' => 12]);
$imageUploader->delete(['image_id' => 42]);

[scootstah]

Sounds like you're trying to make a routing system.

 

Your "task" should be a controller class, and your upload/update/delete should be methods within that controller. A basic example might be:

<?php

class DoSomethingController
{
	public function upload()
	{
		// upload image
	}	

	public function update()
	{
		// upload image and replace an existing one
	}

	public function delete()
	{
		// delete an image
	}
}

You'd then have some routing logic to determine which controller to execute based on the "task" parameter.

[NotionCommotion]

Thanks Jacques1, Guess that makes sense, but will need to give it a bit more thought.  Why do you think doing so is more appropriate than something like the following:

new myImageCropper($arr);

class myImageCropper extends imageCropper
{
   public function update()
   {
      parent::update();
      //SQL to update database goes here
   }
}

[NotionCommotion]

Thanks Scootstah,

 

My original question dealt with with how to keep the base class intact and add additional script to update the database, and not how to implement a router/controller.  Maybe, however, I need help on that part as well.

 

You stated earlier:  Also, for the record, constructors should only be concerned with setting up the class... and not actually doing anything.  Originally I was planning on putting the controller just for tasks associated with this particular class in the constructor and implementing the appropriate task based on a GET or POST value.  Why shouldn't the constructor do something other than set up a class?

[Jacques1]

 

Why do you think doing so is more appropriate than something like the following:

 

Judging from your code example, you want to dynamically add relatively simple actions to the standard update/delete procedure (hence your idea to use a callback).

 

It would be a bit of an overkill to extend the entire class multiple times only to add trivial stuff like a database query. You also won't be able to combine the actions (like updating a database and sending out an e-mail). With the observer pattern, you can keep each extra functionality in a very simple class of its own and simply plug it into the main upload procedure.

 

But you know your application best, so if you find subclasses more appropriate, that may very well be the case.

[scootstah]

Why shouldn't the constructor do something other than set up a class?

Because that is not its job. A class shouldn't do anything until a method is called. Putting business logic in a constructor breaks the Single Responsibility Principle, makes testing harder, and could cause problems with subclasses.

 

My original question dealt with with how to keep the base class intact and add additional script to update the database, and not how to implement a router/controller.

Indeed, but you also said this, which is what a router does:

 

To accompany the plugin, I also wrote a PHP class which has four public methods, and the method to execute is based on $_GET or $_POST['method_to_execute'].

init(). The configuration of the plugin is all defined serverside and the plugin gets the configuration settings from the server when it is initialed.

upload(). Uploads the image and saves it in a temporary folder.

update(). Crops and resizes the image and moves it to a permanent folder.

delete(). Deletes the previously image which is stored in the permanent folder.

[NotionCommotion]

Thank you all!

 

Given more thought, I totally agree that I shouldn't use a constructor to do something.  If the class only has one purpose, why make it happen using a constructor, and instead I should just use a simple function.  Agree?

 

Jacques1 suggestion of using the observer pattern has merit, and I will probably do so.

 

Still curious why you all feel anonymous callback functions are such a bad idea?  The whole purpose of this script is to respond to a jQuery plugin, and typically JavaScript/jQuery often uses anonymous callback functions for fairly sophisticated requirements.  When configuring the server script, it would maybe be nice to follow the same design pattern as when configuring the jQuery plugin.

[scootstah]

Javascript uses prototypal inheritance. It doesn't even have classes, so you can't really compare the two.

 

Using anonymous functions like you are makes unit testing very difficult, makes your code tightly coupled, and doesn't really allow for reusability.

[Jacques1]

There's nothing wrong with closures themselves, but yours are way too complex and have unexpected side effects. The fact that you had to open a thread just for the implementation kind of proves that.

 

Closures should be used as auxiliary functions performing a single simple task. For example, they're perfect for functions like usort() or preg_replace_callback(). But when you start to move your application logic and database queries into a bunch of callbacks, that's too much. When I saw your code for the first time, it immediately felt weird, and it took me a while to figure out what this even does. The heavy lifting should be done by functions and methods, not callbacks.