Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revised file management and new file hooks #7

Closed
wants to merge 4 commits into from

Conversation

nblackman
Copy link

I need to synchronize pimcore's files on multiple app servers without using NFS. After some prior discussion, it seems like adding new hooks would be one approach to this problem. This would allow plugins to interact with pimcore's file lifecycle. For this purpose, I am proposing two hooks:

preFileChange
Called before a file is created, modified, or deleted.
postFileChange
Called after a file is created, modified, or deleted.

The following is an example of how one of the hooks is used in a plugin:

public function postFileChange(Pimcore_Event_File $event) {
    $file = $event->getFile();
    $filetype = get_class($file);
    $filename = $file->getPath();
    $eventType = get_class($event); //Pimcore_Event_File_Created, Modified, or Deleted
}

These hooks could be added around every file action in pimcore like this:

$file = new Pimcore_File($filepath);
$file->dispatchPreEvent(Pimcore_Event::EVENT_TYPE_FILE_MODIFIED);
file_put_contents($filepath, $contents);
$file->dispatchPostEvent(Pimcore_Event::EVENT_TYPE_FILE_MODIFIED);

But this would require adding the same code around every file operation, which seems redundant. Instead, I saw an opportunity for improving pimcore's file management by unifying file operations into the Pimcore_File class. That way, the above code would become:

$file = new Pimcore_File($filepath);
$file->setContents($contents);
$file->save();

Here are a few of the advantages of promoting files to a first class pimcore object:

  1. Less code repetition. For example, almost every file saving operation currently has code similar to the following:

    if(!exists(dirname($destination)) { 
         mkdir(dirname($destination));
    }
    
    file_put_contents($destination, $contents);
    
    chmod($destination, 0766);
    

    In my revision, this logic only has to be maintained once in Pimcore_File->save().

  2. File hooks are called automatically and consistently. The hooks can be dispatched both in core or in plugins simply by manipulating the file.

  3. More flexibility. Centralizing file actions opens more possibilities. For example, it would be easy to log every file operation. Every file action could even be extended to make a custom file driver for something like mogileFS or for copying files to a CDN.

Because I would like feedback before continuing further, so far I have only added the new file management to the following areas in pimcore:

  • Versions
  • Object Classes
  • Fieldcollections
  • Objectbricks
  • Recyclebin
  • Assets

Although some of these might not be necessary, the remaining areas I can think of are:

  • Log files
  • Temporary files
  • Various XML config files
  • Grid view configs
  • WebDAV
  • Image manipulation

Since there may be times when you only need basic file manipulation without the overhead of hooks, I added the ability to disable file events by calling Pimcore_File::disableEvents(), similar to the way versioning can be disabled. I'm thinking it might be useful to also have a system setting to permanently disable file hooks for those who don't need it.

@timglabisch
Copy link

i also think that pimcore should wrap all io operations in special classes. but i dont like the idea of using hooks.
hooks are really expensive and normal times you dont need them. if you allow users to activate or deactive them, may some plugins doesnt work well.

in my opinion the best way would be a special driver like in this example:

https://github.com/timglabisch/pimcore/blob/dependency_injection_anydi/pimcore/lib/Pimcore/Backup.php#L113

for me, data access isn't something else like database access. the database use a driver and the filesystem should use a driver, too. all in all it's also the best for implementing something like a service locator / dependency injection container.

here is a really basic example of the used driver https://github.com/timglabisch/pimcore/blob/dependency_injection_anydi/pimcore/lib/Pimcore/Io.php
i just mapped the standard functions, but there are some problems doing something like that. you also must warp streams, the performance isnt so good and so on. i looked at popular frameworks like symfony, they still just use normal io functions.

why you dont overwrite the file stream wrapper? it's build in, has great performance and it's really easy to write a plugin that can map all IO to something like S3, a Database or whatever you want.

i also cant understand why this helps you to scale. if you have around 50 servers, you cant sync them using hooks. you just will have some ugly master - master trouble.

if you dont have so much servers, you should think about scale using http accelerators like varnish. you can build a complex setup using a lot of varnish server. with around 35 servers you could deliver the stern.tv traffic.

and alternative and cool way could be generating debian packages from the backup and deploying them automatically - thats the reason why i started rewriting the Pimcore_Backup =). so you could scale extrem flexible using unlimit servers.

@nblackman
Copy link
Author

First, I want to thank you for your input. This is a problem I am very interested in solving--it has been plaguing me for a few months. I'm going to respond to your concerns, but I'm still open to suggestions. I am always interested in better solutions.

the database use a driver and the filesystem should use a driver, too.

I agree that a special driver is the best way. In fact, I just committed something similar. It doesn't replace every file operation like yours, but it does abstract a few basic actions like load, save, delete, etc. If a custom file adapter is specified in the system config, a plugin can override these basic file operations without using hooks.

but i dont like the idea of using hooks. hooks are really expensive and normal times you dont need them.

I didn't remove the hooks because I still think they could be useful. Also, these hooks are only for write operations, so the performance difference shouldn't be very noticeable unless you're writing thousands of files at a time. I think hooking file operations is reasonable. For instance, Drupal has hooks for all of its file actions.

if you allow users to activate or deactive them, may some plugins doesnt work well.

Only the preFileChange and postFileChange hooks would be disabled. Any plugin that depends on these hooks could easily require that they be enabled.

why you dont overwrite the file stream wrapper? it's build in, has great performance and it's really easy to write a plugin that can map all IO to something like S3, a Database or whatever you want.

I actually tried doing that a while back and ran into a few problems. The biggest problem is that pimcore is not very stream wrapper friendly right now. Stream wrappers do not support functions like realpath(), glob(), chmod(), imagepng(), etc. A lot of these unsupported functions are used in pimcore. Workarounds would have to be made for all of the current code that uses these functions. Also, stream wrappers are a little more awkward to develop with compared to the nice workflow of pimcore plugins. With that being said, I agree that stream wrappers would have better performance. If there is enough resistance to centralizing pimcore's file management, the alternative would be to make pimcore more compatible with stream wrappers. Then I could write a custom stream wrapper.

i also cant understand why this helps you to scale. if you have around 50 servers, you cant sync them using hooks. you just will have some ugly master - master trouble.

Actually, I can sync them using hooks. I left out some detail about how I would do that. We have a messaging system that manages our cluster of app servers. It already works well, but I need pimcore to be able to talk to it.

@timglabisch
Copy link

thanks for the fast reply, i love to see that pimcore`s community grows up.
At the moment there are 2 different pull requests for pimcore. both requests wants to add new hooks.
in my opinion we should be really careful to make sure that pimcore really just implement really necessary hooks.
if a hook is implemented, you really have to support it that way, so if we implement something like a io hook, may there will be some plogins using that. so we cant implement something like a abstract io driver or something else to make sure, that everything is working well. supporting "quick"-implemented hooks can results in code, which is very hard to refactor.
thats the reason why i'm a bit afraid of every new hook.

i think you has a really special usecase. implementing hooks just for write operation isn't strict. if elements.at would like this idea, i really would prefer implementing it for all file actions.

  1. would you like the idea for implementing a special io driver instead of this hooks?
  2. would a io driver solve all your problems?

just for my personal interest

  1. can you explain me, why you dont use a network file system?
  2. how much servers you have in sync?
  3. do you use RabbitMQ, German?

@nblackman
Copy link
Author

would you like the idea for implementing a special io driver instead of this hooks?

Absolutely. In fact, I already made a special IO driver to make it easier to add these hooks. I made Pimcore_File a wrapper for a swappable file driver class that I called Pimcore_File_Adapter. If you get a chance, take a look at https://github.com/subspacer/pimcore/blob/filedrivers/pimcore/lib/Pimcore/File/Adapter.php and https://github.com/subspacer/pimcore/blob/filedrivers/pimcore/lib/Pimcore/File/Adapter/Disk.php.

File drivers and file hooks should be probably be two separate considerations anyway, so I went ahead and made each of them their own branch. I will probably close this request soon and add a pull request for them individually.

if a hook is implemented, you really have to support it that way, so if we implement something like a io hook, may there will be some plogins using that. so we cant implement something like a abstract io driver or something else to make sure, that everything is working well. supporting "quick"-implemented hooks can results in code, which is very hard to refactor.
thats the reason why i'm a bit afraid of every new hook.

I completely understand being hesitant to add hooks on a whim. They can definitely become excessive quickly. But I think having a quality set of plugin hooks is necessary to remain competitive. After all, the biggest successes (e.g. Drupal, Magento, Wordpress, etc.) have hundreds of them.

implementing hooks just for write operation isn't strict. if elements.at would like this idea, i really would prefer implementing it for all file actions.

I included events for file creation, modification, deletion, and moving. I intentionally skipped file load actions because I didn't want to slow down file reading. What other actions would you want an event for?

i think you has a really special usecase.

My main focus right now is making pimcore more cloud-compatible. I think a lot of people could benefit from this improvement. I find it more unusual that pimcore uses so many files as a datasource, yet has little flexibility regarding their storage. For example, what if you wanted to encrypt all recycle bin or version files? What about realtime backups? A plugin is pretty limited in this way.

would a io driver solve all your problems?

Yes it would. I would be very satisfied if plugins could supply a custom driver. I was mostly considering the file hooks as an alternative that would be less invasive than rewriting all of pimcore's file management.

@timglabisch
Copy link

i think your implemention looks well, but there are missing all functions using a filestream feof, fgets, ...

i dont like the idea of implementing 2 approaches (hooks and driver). may an alternative would be adding a driver which fires hooks by default. if you want you could use such a driver.

the main problem using a driver is, that you just can use one special driver. so a plugin couldnt force them. may an alternative would be register a driver by a plugin, but you still have to configure it in the configuration.

i am really curious about the implementation prefered by elements.at

@nblackman
Copy link
Author

but there are missing all functions using a filestream feof, fgets, ...

I didn't worry much about those right now because pimcore doesn't really use them. I figured I could increase the code coverage later if this type of file handling gets approved. Besides, my goal was to abstract the file manipulation rather than just wrap all of PHP's file functions. This way users only have to worry about simple functions like save and load. I'm also not sure if we would want to force all drivers like an S3 or FTP file driver to support feof or fgets functions.

i dont like the idea of implementing 2 approaches (hooks and driver). may an alternative would be adding a driver which fires hooks by
default. if you want you could use such a driver.

the main problem using a driver is, that you just can use one special driver. so a plugin couldnt force them. may an alternative would be > register a driver by a plugin, but you still have to configure it in the configuration.

If we had both hooks and drivers, a plugin could force the drivers pretty easily. For example:

public function preFileChange(Pimcore_Event_File $event)
{
     $file = $event->getFile();
     if($file instanceof Pimcore_File_Type_Asset) {
          $file->setAdapter('Pimcore_File_Adapter_S3');
     } else {
          $file->setAdapter('Pimcore_File_Adapter_Disk');
     }
}

Without hooks, like you said, this could be done using a configuration file that the plugin would need to modify. Something like:

<?xml version="1.0"?>
<zend-config xmlns:zf="http://framework.zend.com/xml/zend-config-xml/1.0/">
  <files>
    <mapping>
        <filetype>Pimcore_File_Type_Asset</filetype>
        <adapter>Pimcore_File_Adapter_S3</adapter>
    </mapping>
...

i am really curious about the implementation prefered by elements.at

Me too. I'm anxious to hear their thoughts and insight. I created an issue in the issue tracker so hopefully we will hear from them soon.

@nblackman nblackman closed this Jan 11, 2012
brusch pushed a commit that referenced this pull request Dec 17, 2012
Apke added a commit to Apke/pimcore that referenced this pull request Feb 5, 2018
Hi,

Maybe a little bit special, but i got an error because of undefined Service for `monolog.logger.pimcore` while Check Requirements Route.

The Exception:
```
request.CRITICAL: Uncaught PHP Exception Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: "You have requested a non-existent service "monolog.logger.pimcore". Did you mean one of these: "monolog.logger.event", "monolog.logger.php", "monolog.logger.request", "monolog.logger.cache", "monolog.logger.console"?" at /var/www/html/client.local/vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/Container.php line 348 {"exception":"[object] (Symfony\\Component\\DependencyInjection\\Exception\\ServiceNotFoundException(code: 0): You have requested a non-existent service \"monolog.logger.pimcore\". Did you mean one of these: \"monolog.logger.event\", \"monolog.logger.php\", \"monolog.logger.request\", \"monolog.logger.cache\", \"monolog.logger.console\"? at /var/www/html/client.local/vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/Container.php:348)"} []
```

It is caused because of Pimcore\Logger usage of some Methods like `Pimcore\Config::getSystemConfig()`

An Example ExceptionTrace (Working dir is replaced with `./`)
````
#0 ./pimcore/lib/Pimcore/Logger.php(29): Symfony\Component\DependencyInjection\Container->get('monolog.logger....')
#1 ./pimcore/lib/Pimcore/Logger.php(44): Pimcore\Logger::log('Cannot find sys...', 'emergency', Array)
pimcore#2 ./pimcore/lib/Pimcore/Config.php(114): Pimcore\Logger::emergency('Cannot find sys...')
pimcore#3 ./pimcore/lib/Pimcore/Tool/Console.php(80): Pimcore\Config::getSystemConfig()
pimcore#4 ./pimcore/lib/Pimcore/Tool/Console.php(233): Pimcore\Tool\Console::getExecutable('php', true)
pimcore#5 ./pimcore/lib/Pimcore/Tool/Requirements.php(328): Pimcore\Tool\Console::getPhpCli()
pimcore#6 ./pimcore/lib/Pimcore/Bundle/InstallBundle/Controller/InstallController.php(88): Pimcore\Tool\Requirements::checkExternalApplications()
pimcore#7 ./vendor/symfony/symfony/src/Symfony/Component/HttpKernel/HttpKernel.php(151): Pimcore\Bundle\InstallBundle\Controller\InstallController->checkAction(Object(Symfony\Component\HttpFoundation\Request), Object(Pimcore\Install\Installer))
pimcore#8 ./vendor/symfony/symfony/src/Symfony/Component/HttpKernel/HttpKernel.php(68): Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object(Symfony\Component\HttpFoundation\Request), 1)
pimcore#9 ./vendor/symfony/symfony/src/Symfony/Component/HttpKernel/Kernel.php(202): Symfony\Component\HttpKernel\HttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
pimcore#10./web/install.php(72): Symfony\Component\HttpKernel\Kernel->handle(Object(Symfony\Component\HttpFoundation\Request))
pimcore#11 {main}
````

best regards
@Cruiser13 Cruiser13 mentioned this pull request Feb 7, 2018
kudeepak added a commit to kudeepak/pimcore that referenced this pull request Mar 7, 2021
Error displaying at the time of class creation using admin, on line 312. I just make change  if ($data != null && method_exists($data, 'getChildren')) { 


Error was as below : 

Timestamp: Sun Mar 07 2021 09:28:58 GMT+0530 (India Standard Time)
Status: 500 | Internal Server Error
URL: /admin/class/add
Method: POST
Message: method_exists(): Argument #1 ($object_or_class) must be of type object|string, null given
Trace: 
in /var/www/html/vendor/pimcore/pimcore/models/DataObject/ClassDefinition.php:311
#0 /var/www/html/vendor/pimcore/pimcore/models/DataObject/ClassDefinition.php(311): method_exists(NULL, 'getChildren')
#1 /var/www/html/vendor/pimcore/pimcore/models/DataObject/ClassDefinition.php(584): Pimcore\Model\DataObject\ClassDefinition::cleanupForExport(NULL)
pimcore#2 /var/www/html/vendor/pimcore/pimcore/models/DataObject/ClassDefinition.php(372): Pimcore\Model\DataObject\ClassDefinition->generateClassFiles(true)
pimcore#3 /var/www/html/vendor/pimcore/pimcore/bundles/AdminBundle/Controller/Admin/DataObject/ClassController.php(261): Pimcore\Model\DataObject\ClassDefinition->save(true)
pimcore#4 /var/www/html/vendor/symfony/symfony/src/Symfony/Component/HttpKernel/HttpKernel.php(157): Pimcore\Bundle\AdminBundle\Controller\Admin\DataObject\ClassController->addAction(Object(Symfony\Component\HttpFoundation\Request))
pimcore#5 /var/www/html/vendor/symfony/symfony/src/Symfony/Component/HttpKernel/HttpKernel.php(79): Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object(Symfony\Component\HttpFoundation\Request), 1)
pimcore#6 /var/www/html/vendor/symfony/symfony/src/Symfony/Component/HttpKernel/Kernel.php(195): Symfony\Component\HttpKernel\HttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
pimcore#7 /var/www/html/public/index.php(35): Symfony\Component\HttpKernel\Kernel->handle(Object(Symfony\Component\HttpFoundation\Request))
pimcore#8 {main}
@abdelrhmanyosry abdelrhmanyosry mentioned this pull request Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants