Calling Developers!
We are reenergizing our code contribution process! Learn More

What are the Slack Archives?

It’s a history of our time together in the Slack Community! There’s a ton of knowledge in here, so feel free to search through the archives for a possible answer to your question.

Because this space is not active, you won’t be able to create a new post or comment here. If you have a question or want to start a discussion about something, head over to our categories and pick one to post in! You can always refer back to a post from Slack Archives if needed; just copy the link to use it as a reference..

Since you changed the code sniffer

Options
UM4BZSK7T
UM4BZSK7T Posts: 174 🧑🏻‍🚀 - Cadet

https://github.com/spryker/code-sniffer/compare/0.17.6...master Since you changed the code sniffer check for method annotations I can't commit any more. As you can see now our annotations are not accepted anymore and I see no reason why. I fixed the modules version to 0.17.5 and it runs fine again. I guess that is a bug or what should we change on project level?

Comments

  • Unknown
    Options

    The annotations are correct and have been missing so far. A customer based contribution made this now visible.

  • UM4BZSK7T
    UM4BZSK7T Posts: 174 🧑🏻‍🚀 - Cadet
    Options

    Sorry I don't understand your reply. Code sniffer now says the annotations are not correct. And what has been made visible now?

  • Unknown
    Options

    The annotations it will add are correct. You need to execution them the way the tool tells you to.

  • UM4BZSK7T
    UM4BZSK7T Posts: 174 🧑🏻‍🚀 - Cadet
    Options

    Then it will break with any method that is not part of spryker's core.

  • UM4BZSK7T
    UM4BZSK7T Posts: 174 🧑🏻‍🚀 - Cadet
    Options

    And it's simply not correct if the provided objects are from our namespaces

  • Unknown
    Options

    Can u show or screenshot what it would fix it to?

  • UM4BZSK7T
    UM4BZSK7T Posts: 174 🧑🏻‍🚀 - Cadet
    Options

    Sure :)
    So I set everything to the Spryker namepsace

    /**
     * @method \Spryker\Zed\Sales\Business\SalesFacadeInterface getFacade()
     * @method \Spryker\Zed\Sales\Communication\SalesCommunicationFactory getFactory()
     * @method \Spryker\Zed\Sales\SalesConfig getConfig()
     */
    

    And now I get

      50     Call to an undefined method                                      
             Spryker\Zed\Sales\Business\SalesFacadeInterface::orderExists().  
    

    because of

    $isSuccess = !$this->getFacade()->orderExists($quoteTransfer->getOrderReference());
    

    As I wrote above now the problem is that not all methods can be found in the base class from the Spryker namespace.

  • Unknown
    Options

    Ah, sry, I was kind of out of the door already when I answered. I didnt see that you are using a custom non-standard project setup (and namespace).

    With this you need to add some config in order to keep BC, as this was not the default expectation for customer projects.

    https://github.com/spryker/code-sniffer#configure-custom-namespaces

    Someone could try to PR a better auto detection of namespaces maybe. That would also help.

  • UKTSRTD5M
    UKTSRTD5M Posts: 77 🧑🏻‍🚀 - Cadet
    Options

    Or perhaps it can use the namespace-Configurations by default? Or ist there any specific logic that forces these namespaces to differ from the available ones by default?

  • U018XELUZS9
    U018XELUZS9 Posts: 167 🧑🏻‍🚀 - Cadet
    Options

    Could work like so, did not test that yet:

    diff --git a/Spryker/Sniffs/AbstractSniffs/AbstractMethodAnnotationSniff.php b/Spryker/Sniffs/AbstractSniffs/AbstractMethodAnnotationSniff.php
    index 30fa8e4..45a9b3b 100644
    --- a/Spryker/Sniffs/AbstractSniffs/AbstractMethodAnnotationSniff.php
    +++ b/Spryker/Sniffs/AbstractSniffs/AbstractMethodAnnotationSniff.php
    @@ -9,6 +9,7 @@
    
     use PHP_CodeSniffer\Files\File;
     use SlevomatCodingStandard\Helpers\DocCommentHelper;
    +use Spryker\Shared\Config\Config;
    
     abstract class AbstractMethodAnnotationSniff extends AbstractClassDetectionSprykerSniff
     {
    @@ -27,11 +28,6 @@ abstract class AbstractMethodAnnotationSniff extends AbstractClassDetectionSpryk
          */
         protected const LAYER_BUSINESS = 'Business';
    
    -    /**
    -     * @var string
    -     */
    -    public $namespaces = 'Pyz,SprykerEco,SprykerMiddleware,SprykerSdk,Spryker';
    -
         /**
          * @var bool
          */
    @@ -116,7 +112,7 @@ protected function runSniffer(File $phpCsFile, int $stackPointer): void
          */
         protected function getNamespaceForFilename(File $phpCsFile): ?string
         {
    -        $namespaces = explode(',', $this->namespaces);
    +        $namespaces = $this->getNamespaces();
             foreach ($namespaces as $namespace) {
                 if (
                     $this->fileExists(
    @@ -132,6 +128,47 @@ protected function getNamespaceForFilename(File $phpCsFile): ?string
             return null;
         }
    
    +    protected function getNamespaces(): array
    +    {
    +        $namespaces = [];
    +
    +        $this->defineFakeConstants();
    +        try {
    +            $config = new Config();
    +            $namespaces = $config->get('CORE_NAMESPACES');
    +        }
    +        catch (\Exception $exception) {
    +            // Do nothing
    +        }
    +
    +        return $namespaces;
    +    }
    +
    +    protected function defineFakeConstants(): void
    +    {
    +        if (!defined('APPLICATION_ROOT_DIR')) {
    +            define('APPLICATION_ROOT_DIR', __DIR__);
    +        }
    +        if (!defined('APPLICATION_VENDOR_DIR')) {
    +            define('APPLICATION_VENDOR_DIR', APPLICATION_ROOT_DIR . '/vendor');
    +        }
    +        if (!defined('APPLICATION_SOURCE_DIR')) {
    +            define('APPLICATION_SOURCE_DIR', APPLICATION_ROOT_DIR . '/src');
    +        }
    +        if (!defined('APPLICATION')) {
    +            define('APPLICATION', 'FAKE');
    +        }
    +        if (!defined('APPLICATION_ENV')) {
    +            define('APPLICATION_ENV', 'devtest');
    +        }
    +        if (!defined('APPLICATION_STORE')) {
    +            define('APPLICATION_STORE', 'MDE');
    +        }
    +        if (!defined('APPLICATION_CODE_BUCKET')) {
    +            define('APPLICATION_CODE_BUCKET', 'MDE');
    +        }
    +    }
    +
         /**
          * Checks if the '@method' annotation for the specific method already exists
          * in the class. When $strictCheck is set to true, the method also checks
    
  • UM4BZSK7T
    UM4BZSK7T Posts: 174 🧑🏻‍🚀 - Cadet
    Options

    Looks great 😁 maybe create a pull request on GitHub? I can help you doing that against the Spryker repo if you never done that before. That's easy.

  • Unknown
    edited May 2022
    Options

    Yes, please do so.
    We plan on making another bugfix release this week, so it would fit in perfectly.

  • U018XELUZS9
    U018XELUZS9 Posts: 167 🧑🏻‍🚀 - Cadet
    Options

    I can make a PR on Github, I'm already familiar with that since I contributed these changes to 0.17.6

  • U018XELUZS9
    U018XELUZS9 Posts: 167 🧑🏻‍🚀 - Cadet
    Options
  • UM4BZSK7T
    UM4BZSK7T Posts: 174 🧑🏻‍🚀 - Cadet
    Options

  • Unknown
    Options

    Instead of the config and constant overhead I would rather recommend looking at composer json psr4
    What do u think?

  • U01LKKBK97T
    U01LKKBK97T Posts: 287 🧑🏻‍🚀 - Cadet
    edited May 2022
    Options

    Hasn't been released yet, right? 0.17.7 doesn't have it yet it seems.

  • U018XELUZS9
    U018XELUZS9 Posts: 167 🧑🏻‍🚀 - Cadet
    edited May 2022
    Options

    The PR is still open, feel free to have a look at it and finalize it, since I'm not sure if I finish that. I stopped working with Spryker and I'm looking for a new role currently.

  • UM4BZSK7T
    UM4BZSK7T Posts: 174 🧑🏻‍🚀 - Cadet
    Options

    Why is there now a blank line between all constants? Is this supposed to increas readability for some strange reason?

  • Unknown
    Options

    Is there any alternative PR or alike to be finished as improvement towards more autodetection here?