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

Calling Api Interface from Module Install triggers Exception that Area Code is not set #1405

Closed
Vinai opened this issue Jun 24, 2015 · 14 comments
Assignees
Labels
bug report Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Issue: Format is not valid Gate 1 Failed. Automatic verification of issue format is failed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development

Comments

@Vinai
Copy link
Contributor

Vinai commented Jun 24, 2015

I've run into a problem while writing a small module to play some more with the service layer API.
I'm trying to add attribute options via Magento\Eav\Api\AttributeOptionManagementInterface::add().

This fails because no area is set while running bin/magento setup:upgrade.
Not sure if I'm doing anything wrong. I would expect this to work, since the setup class is using part of the "official" exposed interface of the Magento_Eav module.

Here is the call stack as far as I can see.

Magento\Eav\Api\AttributeOptionManagementInterface::add() causes the event catalog_entity_attribute_save_before to be dispatched as follows:

        $attribute->setOption($options);
        try {
            $this->resourceModel->save($attribute);
        } catch (\Exception $e) {
            throw new StateException(__('Cannot save attribute %1', $attributeCode));
        }

The resource model in this case is an instance of Magento\Eav\Model\Resource\Entity\Attribute

This event has an observer registered, namely Magento\Weee\Model\Observer.
What follows now is a series of dependency resolutions by the object manager.

Magento\Weee\Model\Observer depends on Magento\Tax\Helper\Data
Magento\Tax\Helper\Data depends on Magento\Tax\Api\TaxCalculationInterface
The preference for Magento\Tax\Api\TaxCalculationInterface is Magento\Tax\Model\TaxCalculation
Magento\Tax\Model\TaxCalculation is dependent on Magento\Tax\Model\Calculation
Magento\Tax\Model\Calculation is dependent on Magento\Customer\Model\Session
Magento\Customer\Model\Session is dependent on Magento\Framework\Session\Generic
Magento\Framework\Session\Generic extends from \Magento\Framework\Session\SessionManager
Magento\Framework\Session\SessionManager depends on \Magento\Framework\App\State

So far so good, but in the constructor the session manager calls $this->start():

Magento\Framework\Session\SessionManager::__construct() calls $this->start()
Magento\Framework\Session\SessionManager::start() calls Magento\Framework\App\State::getAreaCode()

Here is the method in question:

    public function getAreaCode()
    {
        if (!isset($this->_areaCode)) {
            throw new \Magento\Framework\Exception\LocalizedException(
                new \Magento\Framework\Phrase('Area code is not set')
            );
        }
        return $this->_areaCode;
    }

Now the problem is that $this->_areaCode is not set.

Summary: When running bin/magento the area code is not set on the app state instance.
This causes an exception while the session is started.

Not sure where the bug lies: the session being started while bin/magento is being executed from the command line or the app area not being set.
I can imagine the same problem cropping up in different API service layer calls, too, since this seems to be a low level issue.

Adding the attribute options via the eav setup class, or via attribute models and collections works like a charm, however I'm doing the exercise to practice how to do things the right way and use the service layer instead.

PS: In case it's helpful, I've pushed the code that calls the API service layer to github: https://github.com/vinai/VinaiKopp_EavOptionSetup

If you think it is useful I'd be happy to add the module install that triggers the error to this issue thread, too.

@Vinai Vinai changed the title Calling Api Interface Method triggers session_start Calling Api Interface from Module Install triggers Exception that Area Code is not set Jun 24, 2015
@buskamuza
Copy link
Contributor

@Vinai , good point.
There is a big question about areas in Magento CLI now. We're currently working on it, so your input would be very helpful.
So, I think:

  1. It's questionable, if any CLI may require a session. Because it will never have real session. So I'd expect that any model may work with the session, but it should not be required. Maybe it's even implemented in this way, but it leads to some deep dependencies that lead to the broken application still.
  2. Magento CLI includes different commands and different commands may need different area.
  3. Currently I'm trying to see, if it's possible to make a command developer to define area for his/her command and how it will look like. Another question is whether it makes sense to make some area (for example, "adminhtml") a default one for commands? Or it should be set by the developer explicitly and by default it should be global area?
  4. If we say about "setup:upgrade" command, it, probably, makes sense to run it in "adminhtml" area, as it usually creates entities usually created from Admin Panel. While, in general, during install/upgrade any kind of entities may be created, including ones that are usually created in "frontend" area, so events for such entities would not work.

@Vinai
Copy link
Contributor Author

Vinai commented Jun 25, 2015

Thanks for your reply @buskamuza!

TD;DR
I agree with your assessment.

  • The default area during CLI command execution should be adminhtml
  • Sessions should disabled during CLI command execution
  • Some mechanism to initialize a different area during CLI command execution needs to be provided for commands like cron:run

The following is roughly my thought trail how I came to this opinion.
I've tried to collect my understanding, or lack thereof, of required areas during CLI commands in the following table. I'm using adminhtml as the default area here because it seems like an obvious choice, and only listing another area if I think adminhtml might not work.
So even if technically a command wouldn't require any area to be set, as long as adminhtml would be okay, that's what I've listed in that column.

Command Area Requires Sessions  Comment
admin:user:create adminhtml  no
cache:clean adminhtml  no
cache:disable adminhtml  no
cache:enable adminhtml no
cache:flush adminhtml no
cache:status adminhtml no
cron:run cron no
dev:css:deploy adminhtml no
dev:tests:run adminhtml  no Test have to set the area to whatever fixture is required anyway
dev:xml:convert adminhtml no
i18n:collect-phrases adminhtml no
i18n:pack adminhtml no
i18n:uninstall adminhtml no
indexer:info adminhtml no
indexer:reindex adminhtml no
indexer:set-mode adminhtml no
indexer:show-mode adminhtml  no
indexer:status adminhtml no
info:backups:list adminhtml no
info:currency:list adminhtml no
info:dependencies:show-framework adminhtml no
info:dependencies:show-modules adminhtml  no
info:dependencies:show-modules-circular adminhtml no
info:language:list adminhtml no
info:timezone:list adminhtml no
log:clean adminhtml  no
log:status adminhtml no
maintenance:allow-ips adminhtml no
maintenance:disable adminhtml no
maintenance:enable adminhtml no
maintenance:status adminhtml no
module:disable adminhtml no
module:enable adminhtml  no
module:status adminhtml no
module:uninstall adminhtml no
sampledata:install adminhtml no
setup:backup adminhtml no
setup:config:set adminhtml no
setup:db-data:upgrade adminhtml  no
setup:db-schema:upgrade adminhtml no
setup:db:status adminhtml no
setup:di:compile adminhtml no
setup:di:compile-multi-tenant ? no Not sure how this works or what the area requirements would be
setup:install adminhtml no
setup:performance:generate-fixtures adminhtml  no
setup:rollback adminhtml no
setup:static-content:deploy adminhtml no
setup:store-config:set adminhtml no
setup:uninstall adminhtml no
setup:upgrade adminhtml no
theme:uninstall adminhtml no

Regarding the sessions, none of the current commands require an active one.
Should a custom command ever require access to the sessions, it would surely be possible to work around the limitation with a special case session model or something.

Regarding the area, I think adminhtml does seem like good choice as a default. Commands that require a different area to be set are few and should take care of initializing that area themselves.
For command that can work on a frontend area config files for example, the area set while the command is being executed still should be adminhtml.

If it is documented that CLI scripts default to the adminhtml area, then I think it won't be a big issue if frontend events are not triggered during setup:upgrade execution.
Most entities that are created on the frontend are rarely if ever created in module setups. I would consider this a special case that a plugin author will have to consider.

@buskamuza
Copy link
Contributor

@Vinai , thanks for so detailed proposal!
Some commands don't use the application, so they don't care about any area. About all others, I think, you're right. This is similar to what we considered as a simple option instead of flexibility of any area.

@jamescowie
Copy link

Hello @buskamuza do you have an update or workaround for creating CLI scripts that interact with the application ? Im currently trying to develop some CLI functionality but facing the same issues as @Vinai has described.

@buskamuza
Copy link
Contributor

Hi @jamescowie , for now I'd recommend to use a workaround suggested in #1225 (comment)

@MaxSavich
Copy link
Contributor

Internal ticket MAGETWO-37862

Vinai added a commit to Vinai/VinaiKopp_EavOptionSetup that referenced this issue Jul 14, 2015
@duhon duhon added the Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development label Jun 7, 2016
@vkorotun vkorotun removed the PS label Aug 4, 2016
@PascalBrouwers
Copy link
Contributor

I am experiencing this issue with Magento 2.1.0 for the service layer API Magento\Tax\Api\TaxRuleRepositoryInterface:create() (which @Vinai already described is part of his issue).
I just can't phantom that this bug is over a year old and still has not been fixed! Why should I even try using the server layer API when Magento never fixes it?

@Yonn-Trimoreau
Copy link

+1

@hatimeria-artur-jewula
Copy link
Contributor

Any news in this matter? It's been 2 years and still no area code is set when running bin/magento. Writting any custom command that deals with access to data via repositories (ie. Magento\Sales\Api\OrderRepositoryInterface) is limited as DI in most of them will reach some kind of session. Right now the only workaround is to inject Magento\Framework\App\State object into the command and set area code in try...catch block (to avoid collision with other commands that might try the same).

@magento-engcom-team magento-engcom-team added Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development bug report Dev Tools Issue: Format is not valid Gate 1 Failed. Automatic verification of issue format is failed labels Sep 11, 2017
@magento-engcom-team magento-engcom-team added Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed labels Sep 19, 2017
@btwq
Copy link

btwq commented Feb 21, 2018

Working on this ticket #MLAU18

@btwq btwq self-assigned this Feb 21, 2018
@btwq
Copy link

btwq commented Feb 21, 2018

Spoke with @maksek the agreed approach likely to be manually declaring in execute() in custom class that extends Command:

protected function execute(){ 

    $this->state->setAreaCode(\Magento\Framework\App\Area::AREA_FRONTEND);  // See Area class for other available const

  // .....continue execute...
}

A good approach is for custom class to specifically set area code in which this Command operates in, and only set area code if your class performs data operations using any of the api repository classes

@hatimeria-artur-jewula
Copy link
Contributor

@btwq the problem is if some other command defines area code in constructor you will get exception. so it's better to use

try {
    $this->->state->setAreaCode(\Magento\Framework\App\Area::AREA_FRONTEND);
} catch (\Exception $e) {
    // area code already declared
}

And you can decide if area code is different than what you want either emulate or exit with message

@okorshenko
Copy link
Contributor

okorshenko commented Feb 28, 2018

Hi @btwq thank you for research. Closing the issue

@btwq
Copy link

btwq commented Mar 6, 2018

@hatimeria-artur-jewula we discussed whether the try block is required. It makes sense however you run the risk of declaring a different area code than intended. e.g.

  1. In constructor, setAreaCode(Area::AREA_FRONTEND)
  2. In execute(), setAreaCode(Area::AREA_ADMIN).

If (2) was in try block, then setAreaCode in (2) is ignored, and now you have set area as FRONTEND instead of ADMIN. By avoiding a try block, the developer is then forced to figure out where and what AreaCode is set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug report Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Issue: Format is not valid Gate 1 Failed. Automatic verification of issue format is failed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development
Projects
None yet
Development

No branches or pull requests