Consider using exceptions instead of null values to signal invalid plugins during discovery

Created on 12 July 2024, 9 months ago

Problem/Motivation

While working on 🐛 Changing plugins from annotations to attributes in contrib leads to error if plugin extends from a missing dependency Needs review , it was highligthed that the return value of parseClass() as a structure with NULLs is strange, perhaps it should throw an exception in the failure case instead.

The reason we do it is:

              ['id' => $id, 'content' => $content] = $this->parseClass($class, $fileinfo);

so the return value is always expected in that structure. Need to understand if this is all internal enough that we can change it without BC implications.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

📌 Task
Status

Active

Version

11.0 🔥

Component
Plugin 

Last updated about 4 hours ago

Created by

🇮🇹Italy mondrake 🇮🇹

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @mondrake
  • 🇬🇧United Kingdom joachim

    In some cases, the invalid plugin is expected -- such as the case handled by 🐛 Changing plugins from annotations to attributes in contrib leads to error if plugin extends from a missing dependency Needs review . I thought using exceptions as messages was a bad pattern?

  • 🇬🇧United Kingdom longwave UK

    Well the code just does this:

                  ['id' => $id, 'content' => $content] = $this->parseClass($class, $fileinfo);
    
                  if ($id) {
                    $definitions[$id] = $content;
                    // Explicitly serialize this to create a new object instance.
                    $this->fileCache->set($fileinfo->getPathName(), ['id' => $id, 'content' => serialize($content)]);
                  }
                  else {
                    // Store a NULL object, so the file is not parsed again.
                    $this->fileCache->set($fileinfo->getPathName(), [NULL]);
                  }
    

    So really it's just replacing that $id check with a catch? Or, we also allow parseClass to return just NULL directly instead of an array of NULLs?

  • 🇮🇹Italy mondrake 🇮🇹

    My 0.02$: throwing/catching something like an InvalidPluginClassException would be best - clean, unambiguous, testable

Production build 0.71.5 2024