Personal notepad [Deleted]

duderuud

Well-known member
When to uninstall a modification:
Code:
ErrorException: [E_NOTICE] Undefined variable: tableName in src\addons\lgxNotepad\Setup.php at line 42

    XF::handlePhpError() in src\addons\lgxNotepad\Setup.php at line 42
    lgxNotepad\Setup->uninstall() in src\XF\Admin\Controller\AddOn.php at line 635
    XF\Admin\Controller\AddOn->actionUninstall() in src\XF\Mvc\Dispatcher.php at line 321
    XF\Mvc\Dispatcher->dispatchClass() in src\XF\Mvc\Dispatcher.php at line 248
    XF\Mvc\Dispatcher->dispatchFromMatch() in src\XF\Mvc\Dispatcher.php at line 100
    XF\Mvc\Dispatcher->dispatchLoop() in src\XF\Mvc\Dispatcher.php at line 50
    XF\Mvc\Dispatcher->run() in src\XF\App.php at line 2177
    XF\App->run() in src\XF.php at line 390
    XF::runApp() in admin.php at line 13

\src\addons\lgxNotepad\Setup.php
Code:
    public function uninstall(array $stepParams = [])
    {
        // TODO: Implement uninstall() method.
        $sm = $this->schemaManager();
        $sm->dropTable($tableName);      
    }

Correctly:
Code:
    public function uninstall(array $stepParams = [])
    {
        // TODO: Implement uninstall() method.
        $sm = $this->schemaManager();
        $sm->dropTable('lgx_notepad');      
    }
 
PHP:
<?php
namespace lgxNotepad;

use XF\AddOn\AbstractSetup;
use \XF\Db\Schema\Alter;


class Setup extends AbstractSetup
{
    use \XF\AddOn\StepRunnerInstallTrait;
    use \XF\AddOn\StepRunnerUpgradeTrait;
    use \XF\AddOn\StepRunnerUninstallTrait;
 
    public function install(array $stepParams = [])
    {
        // TODO: Implement install() method.
        $this->db()->query("
            CREATE TABLE IF NOT EXISTS `lgx_notepad`
            (
            `notepad_id` INT(5) NOT NULL AUTO_INCREMENT ,
            `user_id` INT(5) NOT NULL ,
            `subject` VARCHAR(500) NOT NULL ,
            `description` TEXT NOT NULL ,
            `created_at` INT(12) NOT NULL ,
            `edit_at` INT(12) NOT NULL ,
            PRIMARY KEY (`notepad_id`)
            )
            ENGINE = MyISAM;
        ");
     
    }

    public function upgrade(array $stepParams = [])
    {
        // TODO: Implement upgrade() method.
    }

    public function uninstall(array $stepParams = [])
    {
        // TODO: Implement uninstall() method.
        $sm = $this->schemaManager();
        $sm->dropTable("lgx_notepad");    
    }
}
Well ok. Why are traits used if the installation is not used in steps? @Chris D, @Kier, @Mike implemented an analogue scheme manager laravel, and here we write a naked request to create a table.
PHP:
<?php
namespace lgxNotepad\Pub\Controller;

use XF\Mvc\ParameterBag;
use XF\Api\Controller\AbstractController;


class Notepad extends AbstractController
{
    public function actionIndex(ParameterBag $params)
    {
        $db = \XF::db();
        $visitor = \XF::visitor();
        if(!$visitor->hasPermission('general', 'lgxNotepadPermission')){
            return $this->noPermission();
        }
     
        if($params['notepad_id'])
        {
            $notepad_id = $params['notepad_id'];
            $note = $db->fetchRow("SELECT * FROM lgx_notepad WHERE notepad_id= '$notepad_id' AND user_id = '$visitor->user_id' ");
            if(!$note){
                return $this->redirect( $this->buildLink('notepad') );
            }
            $params = [
                'note' => $note,
            ];
            return $this->view('', 'lgx_notepadSingle', $params);
        }
        $entries = $db->fetchAll("SELECT * FROM lgx_notepad WHERE user_id = '$visitor->user_id' ORDER BY edit_at DESC");
        $params = [
            'notes' => $entries,
        ];
        return $this->view('', 'lgx_notepad', $params);
    }

 
    public function actionAdd()
    {
        $db = \XF::db();
        $visitor = \XF::visitor();
        $time = \XF::$time;
        if(!$visitor->hasPermission('general', 'lgxNotepadPermission')){
            return $this->noPermission();
        }
     
        if($this->isPost())
        {
            $input = $this->filter([
                'subject' => 'str',
                'description_html' => 'str',
            ]);
            $subject = $input['subject'];
            $html = $input['description_html'];
            $bbCode = \XF\Html\Renderer\BbCode::renderFromHtml($html);
            $description = \XF::cleanString($bbCode);
            $db->query("INSERT INTO `lgx_notepad` (`notepad_id`, `user_id`, `subject`, `description`, `created_at`, `edit_at`) VALUES (? ,? ,? ,? ,? ,?) ", [NULL, $visitor->user_id, new \XF\PreEscaped($subject), new \XF\PreEscaped($description), $time, $time]);
         
            $last_insert = $db->lastInsertId();
            $note = $db->fetchRow("SELECT * FROM lgx_notepad WHERE notepad_id= '$last_insert'");
            return $this->redirect( $this->buildLink('notepad', $note) );
        }
     
        return $this->view('', 'lgx_notepadAddEdit');
    }
 
 
    public function actionEdit(ParameterBag $params)
    {
        $db = \XF::db();
        $visitor = \XF::visitor();
        $time = \XF::$time;
        if(!$visitor->hasPermission('general', 'lgxNotepadPermission')){
            return $this->noPermission();
        }
     
        $notepad_id = $this->filter('id', 'uint');
        if(!$notepad_id){
            $notepad_id  = $params['notepad_id'];
        }
     
        $note = $db->fetchRow("SELECT * FROM lgx_notepad WHERE notepad_id= '$notepad_id' AND user_id = '$visitor->user_id' ");
        if(!$note)
        {
            return $this->redirect( $this->buildLink('notepad') );
        }
         
        if($this->isPost())
        {
            $input = $this->filter([
                'subject' => 'str',
                'description_html' => 'str',
            ]);
            $subject = $input['subject'];
            $html = $input['description_html'];
            $bbCode = \XF\Html\Renderer\BbCode::renderFromHtml($html);
            $description = \XF::cleanString($bbCode);
            $db->query(" UPDATE `lgx_notepad` SET
            `subject`= ?, `description`= ?, `edit_at` = ? , `user_id` = ? WHERE `notepad_id` = ? ",
            [new \XF\PreEscaped($subject), new \XF\PreEscaped($description), $time, $visitor->user_id, $notepad_id] );
         
            return $this->redirect( $this->buildLink('notepad', $note) );
        }
     
        $params = [
            'note' => $note,
        ];
        return $this->view('', 'lgx_notepadAddEdit', $params);
    }
 
 
    public function actionDelete(ParameterBag $params)
    {
        $db = \XF::db();
        $visitor = \XF::visitor();
        if(!$visitor->hasPermission('general', 'lgxNotepadPermission')){
            return $this->noPermission();
        }
     
        $notepad_id  = $params['notepad_id'];
        $db->query("DELETE FROM lgx_notepad WHERE notepad_id= '$notepad_id' AND user_id='$visitor->user_id'");
        return $this->redirect( $this->buildLink('notepad') );
    }
 
}
Oh my GOD.... Please read https://xenforo.com/xf2-docs/dev/entities-finders-repositories/. Direct queries to the database bypassing the abstraction is a sign of poor tone. By the time the developers again implemented ORM and follow the principle of Data Mapper. Request such as $note = $db->fetchRow("SELECT * FROM lgx_notepad WHERE notepad_id= '$notepad_id' AND user_id = '$visitor->user_id' ");
You can replace
$db->fetchRow("SELECT * FROM lgx_notepad WHERE notepad_id= ? AND user_id = ?", $notepad_id, $visitor->user_id)
You can use the finder depending on the need, but it will be more expensive in memory than a request. But in the form that it is now this . You need to first create an entity and create data on it, and CRUD requests in the controller will disappear. In addition to all this, this code violates the principles of the Model View Controller pattern. The idea is interesting, the implementation is terrible.
 
IMHO this is why XF should really consider to review addon code.
Manual code reviews are prohibitively expensive.

Hopefully I am not going to insult anyone by saying so, but a lot of Add-ons seem to be developed by Part-Time Developers and Amateurs.
They (ideally) do what they are intended to do, but they are not developed with security, performance and maintainability in mind.
I also fear that you might be kinda ... hmm ... disappointed by the outcome of manual checks, that could very well block a significant amount of Add-ons.

But with some effort, automatic checks could be done - there are a lot of amazing things that can be detected by static code analyzers like Phan/PHPStan, phpUnit Tests, etc.

If XenForo would offer some kind of "official CI" with clearly defined rulesets, that would be a major step forward to better code quality.
But that would IMHO alo require clear code style rules, and even XF itself does not a strict code style.
 
@Kirby I have replied in the thread below, as that thread answers some of the things you address:
 
The developer is banned from XF so I cannot release a new version...
Ah ****! I need to contact him. I sent a PM but if he is banned he won't see it. Do you have an e-mail address or something? I want to request a refund for some work he did, but I don't want to go out all-guns-blazing and just dispute the payment without letting him have his say first.

Thanks.
 
there are a lot of amazing things that can be detected by static code analyzers like Phan/PHPStan, phpUnit Tests, etc.
Code scanners often bring up a lot of false positives, but it then becomes the responsibility of the developer to either fix or prove that the finding was false. It would be beneficial for customers to have this added protection before new or modified add-ons are uploaded. I did recently see Chris mention something about XenForo having something in the works (in another thread), though could have misunderstood. It might not be a code scanner, but something is better than nothing.
 
Top Bottom