Update eines Produkts

[Ernestopheles]

Wie update ich ein Isotope Produkt? Bisher habe ich nur den direkten Database Zugriff dafür gefunden; geht es nicht auch mit einer save() Methode?

[BennyBorn]

Du meinst sowas wie (ungetestet)?

$product = Isotope\Model\Product::findById('1');
$product->title = 'test';
$product->save(); 

[Ernestopheles]

Genau, damit erhalte ich dass das Objekt detached sei und daher nicht gespeichert werden könne.

[Spooky]

Poste deinen Code.

[Ernestopheles]

    /**
     * @param Product $objProduct
     * @param string  $inventory_status
     */
    public function updateInventoryStatus($objProduct, $inventory_status): void
    {
        // Update of product inventory status
        $objProduct->inventory_status = $inventory_status;

        $objProduct->save();

        //Todo: check if update was successful?
    } 

Die Methode ist in einer Helper Klasse, die von einem Listener verwendet wird, der wiederum von einem Isotope Hook gerufen wird.


Stacktrace:

RuntimeException:
The model instance has been detached and cannot be saved

at vendor/contao/core-bundle/src/Resources/contao/library/Contao/Model.php:426
at Contao\Model->save()
(/shared/httpd/nahati/contao-isotope-stock/src/Helper/Helper.php:40)
at nahati\ContaoIsotopeStockBundle\Helper\Helper->updateInventoryStatus(object(Standard), '2')
(/shared/httpd/nahati/contao-isotope-stock/src/Helper/Helper.php:170)
at nahati\ContaoIsotopeStockBundle\Helper\Helper->manageStockAndReturnSurplus(object(Standard), '1')
(/shared/httpd/nahati/contao-isotope-stock/src/EventListener/UpdateItemInCollectionListener.php:113)
at nahati\ContaoIsotopeStockBundle\EventListener\Upda teItemInCollectionListener->__invoke(object(ProductCollectionItem), array('quantity' => '1'), object(Cart))
(vendor/isotope/isotope-core/system/modules/isotope/library/Isotope/Model/ProductCollection.php:1201)
at Isotope\Model\ProductCollection->updateItemById(3094, array('quantity' => '1'))
(vendor/isotope/isotope-core/system/modules/isotope/library/Isotope/Module/AbstractProductCollection.php:219)
at Isotope\Module\AbstractProductCollection->updateItemTemplate(object(Cart), object(ProductCollectionItem), array('id' => 3094, 'sku' => 'S0002', 'name' => 'Erstickungsgefährdet', 'options' => array('status' => array('label' => 'Status', 'value' => 'Original')), 'configuration' => array('status' => object(Plain)), 'attributes' => array('status' => 1), 'quantity' => 1, 'price' => '0,00&nbsp;<span class="currency">€</span>', 'tax_free_price' => '0,00&nbsp;<span class="currency">€</span>', 'original_price' => '0,00&nbsp;<span class="currency">€</span>', 'total' => '0,00&nbsp;<span class="currency">€</span>', 'tax_free_total' => '0,00&nbsp;<span class="currency">€</span>', 'original_total' => '0,00&nbsp;<span class="currency">€</span>', 'tax_id' => '', 'href' => 'kunstwerke/skulpturen/erstickungsgef%C3%A4hrdet.html?status=1', 'hasProduct' => true, 'product' => object(Standard), 'item' => object(ProductCollectionItem), 'raw' => array('id' => 3094, 'pid' => 261, 'tstamp' => 1686229091, 'product_id' => 45, 'type' => 'standard', 'sku' => 'S0002', 'name' => 'Erstickungsgefährdet', 'configuration' => 'a:1:{s:6:"status";i:1;}', 'quantity' => 1, 'price' => '0.00', 'tax_free_price' => '0.00', 'tax_id' => '', 'jumpTo' => 33), 'rowClass' => 'product row_0 row_even row_first'), array('1'), true)
(vendor/isotope/isotope-core/system/modules/isotope/library/Isotope/Module/Cart.php:113)
at Isotope\Module\Cart->updateItemTemplate(object(Cart), object(ProductCollectionItem), array('id' => 3094, 'sku' => 'S0002', 'name' => 'Erstickungsgefährdet', 'options' => array('status' => array('label' => 'Status', 'value' => 'Original')), 'configuration' => array('status' => object(Plain)), 'attributes' => array('status' => 1), 'quantity' => 1, 'price' => '0,00&nbsp;<span class="currency">€</span>', 'tax_free_price' => '0,00&nbsp;<span class="currency">€</span>', 'original_price' => '0,00&nbsp;<span class="currency">€</span>', 'total' => '0,00&nbsp;<span class="currency">€</span>', 'tax_free_total' => '0,00&nbsp;<span class="currency">€</span>', 'original_total' => '0,00&nbsp;<span class="currency">€</span>', 'tax_id' => '', 'href' => 'kunstwerke/skulpturen/erstickungsgef%C3%A4hrdet.html?status=1', 'hasProduct' => true, 'product' => object(Standard), 'item' => object(ProductCollectionItem), 'raw' => array('id' => 3094, 'pid' => 261, 'tstamp' => 1686229091, 'product_id' => 45, 'type' => 'standard', 'sku' => 'S0002', 'name' => 'Erstickungsgefährdet', 'configuration' => 'a:1:{s:6:"status";i:1;}', 'quantity' => 1, 'price' => '0.00', 'tax_free_price' => '0.00', 'tax_id' => '', 'jumpTo' => 33), 'rowClass' => 'product row_0 row_even row_first'), array('1'), true)
(vendor/isotope/isotope-core/system/modules/isotope/library/Isotope/Module/AbstractProductCollection.php:102)
at Isotope\Module\AbstractProductCollection->compile()
(vendor/isotope/isotope-core/system/modules/isotope/library/Isotope/Module/Cart.php:53)
at Isotope\Module\Cart->compile()
(vendor/contao/core-bundle/src/Resources/contao/modules/Module.php:214)
at Contao\Module->generate()
(vendor/codefog/contao-haste/library/Haste/Frontend/AbstractFrontendModule.php:52)
at Haste\Frontend\AbstractFrontendModule->generate()
(vendor/isotope/isotope-core/system/modules/isotope/library/Isotope/Module/Module.php:115)
at Isotope\Module\Module->generate()
(vendor/isotope/isotope-core/system/modules/isotope/library/Isotope/Module/AbstractProductCollection.php:59)
at Isotope\Module\AbstractProductCollection->generate()
(vendor/contao/core-bundle/src/Resources/contao/elements/ContentModule.php:98)
at Contao\ContentModule->generate()
(vendor/contao/core-bundle/src/Resources/contao/library/Contao/Controller.php:621)
at Contao\Controller::getContentElement(object(Conten tModel), 'main')
(vendor/contao/core-bundle/src/Resources/contao/modules/ModuleArticle.php:197)
at Contao\ModuleArticle->compile()
(vendor/contao/core-bundle/src/Resources/contao/modules/Module.php:214)
at Contao\Module->generate()
(vendor/contao/core-bundle/src/Resources/contao/modules/ModuleArticle.php:70)
at Contao\ModuleArticle->generate(false)
(vendor/contao/core-bundle/src/Resources/contao/library/Contao/Controller.php:549)
at Contao\Controller::getArticle(object(ArticleModel) , false, false, 'main')
(vendor/contao/core-bundle/src/Resources/contao/library/Contao/Controller.php:391)
at Contao\Controller::getFrontendModule('0', 'main')
(vendor/contao/core-bundle/src/Resources/contao/pages/PageRegular.php:190)
at Contao\PageRegular->prepare(object(PageModel))
(vendor/contao/core-bundle/src/Resources/contao/pages/PageRegular.php:60)
at Contao\PageRegular->getResponse(object(PageModel), true)
(vendor/contao/core-bundle/src/Resources/contao/controllers/FrontendIndex.php:320)
at Contao\FrontendIndex->renderPage(object(PageModel))
(vendor/symfony/http-kernel/HttpKernel.php:163)
at Symfony\Component\HttpKernel\HttpKernel->handleRaw(object(Request), 1)
(vendor/symfony/http-kernel/HttpKernel.php:75)
at Symfony\Component\HttpKernel\HttpKernel->handle(object(Request), 1, true)
(vendor/symfony/http-kernel/Kernel.php:202)
at Symfony\Component\HttpKernel\Kernel->handle(object(Request))
(public/index.php:44)

[Ernestopheles]

use Isotope\Model\Product; 

[Ernestopheles]

Debug zeigt:
Der save() führt direkt in Model::save() und Model::blnPreventSaving ist true, daher die Exception.

Ich verstehe nicht ganz die Zusammenhänge.

Habe es auch mit

Standard 

statt

Product 

versucht (da Product abstrakt ist), mit dem gleichen Resultat.

[Spooky]

Die Instanz, die dir da übergeben wird, kannst du nicht speichern. Du könntest dir aber eine neue Instanz holen.

[Ernestopheles]

Die Instanz, die dir da übergeben wird, kannst du nicht speichern. Du könntest dir aber eine neue Instanz holen.

Warum kann ich das nicht?

Warum sollte es mit einer neuen Instanz gehen?

[Ernestopheles]

Mit

        $existingProduct = Product::findById($objProduct->id);        $existingProduct->save(); 

geht es nicht.

[Ernestopheles]

So lautet der funktionierende Code:

/**     @param Product $objProduct  
  *     @param string  $inventory_status  
*/    
public function updateInventoryStatus($objProduct, $inventory_status): void    {    
  
     // To save the product we need to get a new instance of the product   
     $newProduct = clone Product::findById($objProduct->id);       
     $newProduct->inventory_status = $inventory_status;   
     $newProduct->save();

     //Todo: check if update was successful?    } 

[Ernestopheles]

Leider greift die Lösung so noch nicht. Denn das ursprüngliche Produkt wird nicht upgedatet, es entsteht nur ein neues Produkt (das die geänderten Eigenschaften hat).

Ich möchte das Produkt updaten ohne eine Database Query zu verwenden. So wie es beispielsweise auch bei ProduktCollectionItems geht.

[andreas.schempp]

Isotope Produkte (Models) – genauso wie DC_Multilingual – können nicht mit `save()` gespeichert werden. Die Produkte enthalten nicht die Daten der Datenbank, sondern eine Repräsentation des Produkts, was aus vererbten Daten etc. bestehen kann. Ein speichern würde ggf. zu ungewollten Änderungen führen. Am einfachsten machst du das direkt über eine Datenbankabfrage, und aus meiner Sicht müsstest du dafür auch Lockings verwenden um sicherzustellen dass du keine Race Conditions hast!

[Ernestopheles]

Ich habe mir das genauer angesehen und konnte der Diskussion https://github.com/contao/core/issues/6506 zumindest eine Ahnung entnehmen, was dahintersteckt, dass der save des models verhindert wird. Warum es im konkreten Fall so ist bzw. ob es generell verhindert wird, und das tiefere Verständnis dessen wurde mir nicht klar.
Seis drum.

Meine Lösung ist jetzt diese:

/**     * @param string   $quantity;
     * @param Standard $objProduct;
     * */
    private function updateQuantity($objProduct, $quantity): void
    {
        // To save the product we need to get a new instance of the product
        // see https://github.com/contao/core/issues/6506


        // Get an adapter for the Standard class
        $adapter = $this->framework->getAdapter(Standard::class);


        // Get a new instance of the product
        $objProduct1 = $adapter->findPublishedByPk($objProduct->id);


        // Saving the object like so assumes that there have not been any other changes to the given object. This proves correct if we look at the whole context where this method is used. But it is not so good a solution, as generally methods should be agnostic.
        // An idea how to solve this is to compare the relevant properties of the given instance to the new instance. If they are equal, we can save the updated product. If not, we write a message to the user that the product has changed: 
        if ($objProduct1->quantity == $objProduct->quantity && $objProduct1->inventory_status == $objProduct->inventory_status) {
            $objProduct1->quantity = $quantity;
            $objProduct1->save();
        } else {
            Message::addError(sprintf(
                $GLOBALS['TL_LANG']['ERR']['productHasChanged'],
                $objProduct->getName() ?: $adapter->findPublishedByPk($objProduct->pid)->getName()
            ));
        };


        // TODO: check this solution with the Contao team!

[Ernestopheles]

Isotope Produkte (Models) – genauso wie DC_Multilingual – können nicht mit `save()` gespeichert werden. Die Produkte enthalten nicht die Daten der Datenbank, sondern eine Repräsentation des Produkts, was aus vererbten Daten etc. bestehen kann. Ein speichern würde ggf. zu ungewollten Änderungen führen. Am einfachsten machst du das direkt über eine Datenbankabfrage, und aus meiner Sicht müsstest du dafür auch Lockings verwenden um sicherzustellen dass du keine Race Conditions hast!

Hab deine Antwort erst jetzt gesehen. Sehe ich mir gleich an.

[Ernestopheles]

Hm ja, jedoch wollte ich die DB Abfrage ja gerade vermeiden. Wenn ich es richtig sehe, passt meine Lösung aber ganz gut, da die Schwierigkeiten, die du nennst, hier nicht auftreten können. Oder?

[andreas.schempp]

Hm ja, jedoch wollte ich die DB Abfrage ja gerade vermeiden. Wenn ich es richtig sehe, passt meine Lösung aber ganz gut, da die Schwierigkeiten, die du nennst, hier nicht auftreten können. Oder?

warum nicht? Die Models machen kein Table Locking.

[Ernestopheles]

Wenn ich tstamp abgleiche, sollte ich sicher gehen, auf einen unveränderten Stand upzudaten, oder?

/**     * @paramstring   $inventory_status;
     * @paramStandard $objProduct;
     */
    public function updateInventoryStatus($objProduct, $inventory_status): void
    {
        // To save the product we need to get a new instance of the product
        // see https://github.com/deployphp/deployer/blob/dea01e1dc919c4354dfdff7595b7eec161edece9/projects/contao413/vendor/contao/core-bundle/src/Resources/contao/models/PageModel.php#L1322

        // Get an adapter for the Standard class
        $adapter = $this->framework->getAdapter(Standard::class);

        // Get a new instance of the product
        $objProduct1 = $adapter->findPublishedByPk($objProduct->id);

        // Saving the object like so assumes that there have not been any other changes to the given object. But it is not so good a solution, as generally methods should be agnostic.

        // We check that the product has not been changed in the meantime by comparing tstamp, quantity and inventory_status
        if ($objProduct1->quantity === $objProduct->quantity && $objProduct1->inventory_status === $objProduct->inventory_status && $objProduct1->tstamp === $objProduct->tstamp) {
            $objProduct1->inventory_status = $inventory_status;
            $objProduct1->save();
        } else {
            Message::addError(sprintf(
                $GLOBALS['TL_LANG']['ERR']['productHasChanged'],
                $objProduct->getName() ?: $adapter->findPublishedByPk($objProduct->pid)->getName()
            ));
        }

        // TODO: check this solution with the Contao team!
    }

Sind diese Randbedingungen erfüllt?:
1. findPublishedByPk() holt immer den letzten Stand aus der Datenbank
2. tstamp wird bei jedem update der DB aktualisiert

[andreas.schempp]

nein das funktioniert so nicht, du solltest Table Locking nutzen.
https://learntutorials.net/de/mysql/...l-sperrtabelle

[Ernestopheles]

Ich versuche es zu verstehen. Im Tutorial lese ich:

" Das Sperren ist nur erforderlich, wenn eine Transaktion ausgeführt wird, die zuerst einen Wert aus einer Datenbank liest und diesen Wert später in die Datenbank schreibt. Sperren sind niemals für in sich geschlossene Einfügungs-, Aktualisierungs- oder Löschvorgänge erforderlich."

Bei Onlinetransaktionen im HOST Bereich haben wir es damals so gemacht (DB2 als DB System):
Lese Tabellenzeile mit Sperre für Update
Gebe das auf der Maske aus
Der Benutzer ändert etwas an dieser Zeile und drückt auf Speichern
Lese Tabellenzeile und vergleiche den Timestamp. Wenn unterschiedlich, Meldung an den Benutzer, dass sich der Datenstand geändert hat.
Wenn identisch, Update.

In unserem Fall ist aber keine Benutzertransaktion zwischen Lesen und Update (und es gibt auch nicht wahnsinnig viele Bearbeitungsvorgänge wie in dem Versicherungskonzern), sondern es folgt auf dem Server unmittelbar nacheinander. Deshalb sollte imho das locking unnötig sein. Natürlich könnte auch hier in den Nanosekunden zwischen read und write eine andere Transaktion dazwischenhauen... Ist es denn so, dass in Contao "normalerweise" immer gelockt wird? Man muss ja auch die Nachteile (Performance) bedenken. Alles in allem glaube ich nach meinem jetzigen Erkenntnisstand, dass ich nicht ganz falsch liegen kann mit meiner Lösung. Lerne jedoch immer gerne dazu.

[andreas.schempp]

Aus meiner Sicht besteht immer das Risiko, dass zwei Besucher:innen gleichzeitig ein Produkt bestellen. Für den Lagerbestand wäre es fatal, wenn dieser nicht stimmt. Eine Alternative zum Locking wäre, ein "in sich geschlossenes" Update zu fahren.

// Schlecht
$bestand = $produkt->bestand - $bestellmenge;
$db->prepare("UPDATE tl_iso_product SET bestand=? WHERE id=?")->execute($bestand, $produkt->id);

// Richtig
$db->prepare("UPDATE tl_iso_product SET bestand = (bestand - $bestellmenge) WHERE id=?")->execute($produkt->id); 

[Ernestopheles]

Hm, so ganz überzeugt bin ich zwar nicht, übernehme aber gerne deine Strategie, indem ich auf den save() verzichte und statt dessen einen SQL absetze:

/**     * @paramstring   $inventory_status;
     * @paramStandard $objProduct;
     */
    public function updateInventoryStatus($objProduct, $inventory_status): void
    {
        // Get an adapter for the Database class
        $adapter = $this->framework->getAdapter(Database::class);

        $objProduct1 = $adapter->findPublishedByPk($objProduct->id);

        // We check if relevant properties of the product have been changed in the meantime 
        if ($objProduct1->quantity === $objProduct->quantity && $objProduct1->inventory_status === $objProduct->inventory_status) {

            // no changes -> update inventory_status
            $adapter->getInstance()->prepare('UPDATE tl_iso_product SET inventory_status=? WHERE id=?')->execute($inventory_status, $objProduct->id);
        } else {

            // Changes -> error message
            Message::addError(sprintf(
                $GLOBALS['TL_LANG']['ERR']['productHasChanged'],
                $objProduct->getName() ?: $adapter->findPublishedByPk($objProduct->pid)->getName()
            ));
        }
    }

Der Update sollte in diesem Fall geschlossen genug sein? Könnte man noch strikter gestalten, indem wir der Methode noch den erwarteten alten Stand von inventory_status mitgeben und das update über eine where Bedingung darauf einschränken.

[Ernestopheles]

Jetzt auch mit Lock

Sollte das so jetzt ok sein?

/**     * @paramstring   $inventory_status
     * @paramStandard $objProduct
     */
    public function updateInventoryStatus($objProduct, $inventory_status): void
    {
        // By using adapters we can 1. decouple the dependencies on external classes and 2. we can make use of testdouble adapter in the Unit tests.

        // Get an adapter for the Standard class
        $standardAdapter = $this->framework->getAdapter(Standard::class);

        $objProductDouble = $standardAdapter->findPublishedByPk($objProduct->id);

        // Get an adapter for the Database class
        $databaseAdapter = $this->framework->getAdapter(Database::class);

        // We check if relevant properties of the product have been changed in the meantime 
        if ($objProductDouble->quantity === $objProduct->quantity && $objProductDouble->inventory_status === $objProduct->inventory_status) {

            // no changes -> update inventory_status
            $databaseAdapter->getInstance()->prepare('SELECT*FROM tl_iso_product WHERE id=? FOR UPDATE')->execute($objProduct->id);
            $databaseAdapter->getInstance()->prepare('UPDATE tl_iso_product SET inventory_status=? WHERE id=?')->execute($inventory_status, $objProduct->id);
        } else {

            // Get an adapter for the Message class
            $messageAdapter = $this->framework->getAdapter(Message::class);

            // Changes -> error message
            $messageAdapter->addError(sprintf(
                $GLOBALS['TL_LANG']['ERR']['productHasChanged'],
                $objProduct->getName() ?: $standardAdapter->findPublishedByPk($objProduct->pid)->getName()
            ));
        }
    }

[ErnestophelesDerZweite]

Leider hat die Umstellung von save() auf dem model zu DB Queries den Nachteil, dass die Unit Tests nun nicht mehr das direkte Überprüfen durch Vergleich alt - neu ermöglichen ob ein Update erfolgreich war. Denn die Datenbank Methoden prepare() und execute() müssen gemockt werden und geben deshalb immer das gleiche Ergebnis, während die save() Methode nicht gemockt werden musste.