Fixed deleteWidget in AbstractSetup does not appear to delete the MasterTitle phrase

[DragonByte Tech]

Affected version

2.0.2

Calling $this->deleteWidget($widgetKey); in the uninstaller leaves orphan phrases:



EDIT: Just to confirm, it does actually delete the widget correctly, just not the phrase.


Fillip

 

[Chris D]

I've just tested this with the add-on below and every time the widget is deleted.

 

[DragonByte Tech]

I've just tested this with the add-on below and every time the widget is deleted.

Can you please test it with the eCommerce mod I attached in the conversation? It's the only mod I investigated this with, but my DB is full of imported data and it takes a long time to re-populate

EDIT: Also, just to be sure, the widget always deletes successfully, but the phrases shown in the screenshot did not (for me).


Fillip

 

[Chris D]

Can repro it with your add-on now

 

[DragonByte Tech]

Can repro it with your add-on now

See, I'm not crazy!

If you notice in the Setup.php file, I split a lot of phrase deletions into separate installer steps. I received multiple execution timeout errors if I tried to delete all orphan content phrases in one go due to the DevOutput file deletion, so maybe it's related to that?


Fillip

 

[Chris D]

I don't think that's related, no. I've been doing the installs/uninstalls of your add-on using the CLI so that shouldn't be susceptible to any timeouts.

I notice that one of the widgets does have its phrase deleted and that's the member stat one - so the only one that uses a default widget definition (which isn't deleted during uninstall of course) works as expected.

I sort of pre-empted that could be a factor but that's why I tested that scenario in the test add-on above.

Either way, the Widget Definition entity deletes any widget entities which use that definition, and the deleteWidget method uses the entity to delete so either way I look at it, the master phrase should get deleted.

 

[DragonByte Tech]

Either way, the Widget Definition entity deletes any widget entities which use that definition, and the deleteWidget method uses the entity to delete so either way I look at it, the master phrase should get deleted.

The plot, like me when I look at a piece of bread, thickens.

It appears as if the MasterTitle phrase entity is flagged as deleted:

        if ($this->_deleted)
        {
            die('already deleted');
            return true;
        }

Will continue to dig deeper.


Fillip

 

[DragonByte Tech]

@Chris D Upon further inspection, this was actually due to a bug in the Setup.php file, but the subsequent cause is actually this block of code:

    public function createWidget($widgetKey, $definitionId, array $config, $title = '')
    {
        /** @var \XF\Entity\Widget $widget */
        $widget = $this->app->em()->create('XF:Widget');
        $widget->widget_key = $widgetKey;
        $widget->definition_id = $definitionId;
        $widget->bulkSet($config);
        $widget->save(false);

        $masterTitle = $widget->getMasterPhrase();
        $masterTitle->phrase_text = $title;
        $masterTitle->save(false);
    }

If the widget fails to insert for whatever reason, the phrase is still created.

Might I suggest the following:

    public function createWidget($widgetKey, $definitionId, array $config, $title = '')
    {
        /** @var \XF\Entity\Widget $widget */
        $widget = $this->app->em()->create('XF:Widget');
        $widget->widget_key = $widgetKey;
        $widget->definition_id = $definitionId;
        $widget->bulkSet($config);
        $success = $widget->save(false);
        
        if ($success)
        {
            $masterTitle = $widget->getMasterPhrase();
            $masterTitle->phrase_text = $title;
            $masterTitle->save(false);
        }
    }

That would take care of the issue where an incorrect widget configuration would still generate a phrase that can never be removed


Fillip

 

[Chris D]

Yeah that's wise.