[1.x] Real Estate Manager

Thank you for applying!

Please read Review process for security advisory coverage: What to expect → for more details and Security advisory coverage application checklist → to understand what reviewers look for. Tips for ensuring a smooth review → gives some hints for a smoother review.

The important notes are the following.

• If you have not done it yet, you should run phpcs --standard=Drupal,DrupalPractice on the project, which alone fixes most of what reviewers would report.
• For the time this application is open, only your commits are allowed.
• The purpose of this application is giving you a new drupal.org role that allows you to opt projects into security advisory coverage, either projects you already created, or projects you will create. The project status won't be changed by this application and no other user will be able to opt projects into security advisory policy.
• We only accept an application per user. If you change your mind about the project to use for this application, or it is necessary to use a different project for the application, please update the issue summary with the link to the correct project and the issue title with the project name and the branch to review.

To the reviewers

Please read How to review security advisory coverage applications → , Application workflow → , What to cover in an application review → , and Tools to use for reviews → .

The important notes are the following.

• It is preferable to wait for a Code Review Administrator before commenting on newly created applications. Code Review Administrators will do some preliminary checks that are necessary before any change on the project files is suggested.
• Reviewers should show the output of a CLI tool → only once per application.
• It may be best to have the applicant fix things before further review.

For new reviewers, I would also suggest to first read In which way the issue queue for coverage applications is different from other project queues → .

As a side note, please check you set up Git with the same email registered for your drupal.org account → . Differently, as it is happening, the commits done on any drupal.org repository is not associated with your account.

Thanks @apaderno for pointing that. Of course i can change email and name in local Git settings, but what should i do with my projects repo now? I can't push full code again cause it will not see any changes and it will not update users data.

Commits already done will not be associated to your account, but future commits will be. That's fine; the important is getting the commits associated to your account.

Yes, but according to https://www.drupal.org/node/1011698#s-prerequisites → i have to be author of at least most of this code. The code is 100% mine but commits meta shows different email so as You mention they are not associated to my account.

I'll be watch out for this in the future, in any commits, but question is - Will this code be recognized as mine within this application or do I need to add this code as mine again? For example, adding a commit where I remove all the code, then adding a commit where I add it again with my email, is this the correct way? I don't want to mess things up..

Additionally, I wanted to ask - if the code is not assigned to me, will I have any difficulties in the future somewhere on Drupal or will I not get any credits from it even if I am projects maintainer? Maybe it's better on the start reassigned the code to me and how to do it proper?

My previous comment was just a side note. It was just to notice you that Git was not set with the same email used for your account on drupal.org and you will not see the commits you have done associated to your account, on https://git.drupalcode.org/project/re_mgr/-/commits/1.x.

It does not influence this application, since you are the only project maintainer. Eventually, on issues created for the project, you will not see any link associated to your account, when a commit is shown in an issue comment.

Thank You @apaderno for tip!

Fix phpcs issues.

phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml re_mgr/

FILE: /home/vishalkadam/DRUPAL-REVIEW/re_mgr/modules/visualization/re_mgr_visualization.routing.yml
----------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
----------------------------------------------------------------------
  7 | WARNING | Open page callback found, please add a comment before the line why there is no access restriction
 35 | WARNING | Open page callback found, please add a comment before the line why there is no access restriction
 63 | WARNING | Open page callback found, please add a comment before the line why there is no access restriction
----------------------------------------------------------------------


FILE: /home/vishalkadam/DRUPAL-REVIEW/re_mgr/modules/presentation/re_mgr_presentation.routing.yml
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 6 | WARNING | Open page callback found, please add a comment before the line why there is no access restriction
----------------------------------------------------------------------


FILE: /home/vishalkadam/DRUPAL-REVIEW/re_mgr/re_mgr.routing.yml
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 49 | WARNING | Open page callback found, please add a comment before the line why there is no access restriction
----------------------------------------------------------------------


FILE: /home/vishalkadam/DRUPAL-REVIEW/re_mgr/re_mgr.libraries.yml
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 14 | ERROR | [x] Expected 1 newline at end of file; 0 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: /home/vishalkadam/DRUPAL-REVIEW/re_mgr/css/toolbar-gin.css
-----------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 4 LINES
-----------------------------------------------------------------------
 6 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
 7 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
 8 | ERROR | [x] Expected 1 newline at end of file; 2 found
 9 | ERROR | [x] Additional whitespace found at end of file
-----------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-----------------------------------------------------------------------


FILE: /home/vishalkadam/DRUPAL-REVIEW/re_mgr/css/toolbar.css
-----------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
-----------------------------------------------------------------------
 6 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
 7 | ERROR | [x] Expected 1 newline at end of file; 2 found
 8 | ERROR | [x] Additional whitespace found at end of file
-----------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-----------------------------------------------------------------------


FILE: re_mgr/css/entity-form.css
------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
------------------------------------------------------------------------
 18 | ERROR | [x] Line indented incorrectly; expected 0 spaces, found 2
 19 | ERROR | [x] Line indented incorrectly; expected 0 spaces, found 2
 53 | ERROR | [x] Line indented incorrectly; expected 0 spaces, found 2
------------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------

@vishal.kadam please check new commit i fixed the phpcs errors. Thank You also for tip because i didn't know that phpcs can check also .yml files. Also i have no idea why phpcs didn't showed me those css files before i started this application.

What next guys? ;)

@Mattixonix: Please fix the following.

Use $this->t() instead of t()

src/Entity/Building/Building.php

src/Entity/Estate/Estate.php

src/Entity/Flat/Flat.php

src/Entity/Floor/Floor.php

/** @var array $fields */
    $fields['uid']
      ->setLabel($this->t('Author'))
      ->setDescription($this->t('The Name of the associated user.'))
      ->setSetting('handler', 'default')
      ->setRevisionable(TRUE)
      ->setDisplayOptions('view', [
        'label' => 'above',
        'type' => 'entity_reference_label',
        'region' => 'hidden',
      ])

Add hook_help() for the following module files

modules/demo/re_mgr_demo.module

modules/presentation/re_mgr_presentation.module

modules/visualization/re_mgr_visualization.module

@harishh look at new version i've added help pages to all submodules.

But $this->t() can't be used in static methods like this ::baseFieldDefinitions()

Hi @Mattixonix

Thx a lot for your contribution to the Drupal community. A great respect from me. I did a smoke testing and a review of your module and here are some comments:

• Please, update the README.md file. Here is an example of how to write it properly: https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or... →
• I am not too sure about the gin library support. I've checked the CSS toolbar-gin.css and toolbar.css. I think you can remove the toolbar-gin.css library as your module should be more generic, not somehow attached to an addon theme like gin. Also, you don't require that theme.
• What is the purpose of this file re_mgr.api.php? You don't have any defined logic there. You can remove it.
• Regarding the core version requirement, I am not too sure whether to keep it only for D10. You can easily change into core_version_requirement: ^9 || ^10 for the module and sub-modules too
• You can also have some optimization here for example (file CreatorService):
  

  if (str_starts_with($building_image_name, 'estate')) {
    $name = strtoupper(substr($building_image_name, -1));
    $estate_id = [
      'target_id' => $this->entitiesIdsMap['estate']['estate_id'],
    ];
    $status = self::BUILDINGS_STATUS_MAP[$building_image_name];
  }
  else {
    $name = $this->t('Cool house');
    $estate_id = [];
    $status = NULL;
  }
  

  into:

  $name = $this->t('Cool house');
  $estate_id = [];
  $status = NULL;
  if (str_starts_with($building_image_name, 'estate')) {
    $name = strtoupper(substr($building_image_name, -1));
    $estate_id = [
      'target_id' => $this->entitiesIdsMap['estate']['estate_id'],
    ];
    $status = self::BUILDINGS_STATUS_MAP[$building_image_name];
  }
  

  Or, for example, this one:

  foreach ($this->entitiesIdsMap['estate'][$estate_building_name] as $floor) {
    if (!is_int($floor)) {
      $entity_data['floor_id'] = [
        'target_id' => $floor['floor_id'],
      ];
      for ($i = 0; $i < 4; $i++) {
        $entity_data['name'] = $flat_number;
        $flat_number++;
        $entity_data['status'] = rand(1, 3);
  
        $flat = Flat::create($entity_data);
        $flat->save();
      }
    }
  }
  

  to change into such a statement:

  foreach ($this->entitiesIdsMap['estate'][$estate_building_name] as $floor) {
    if (is_int($floor)) {
      continue;
    }
    $entity_data['floor_id'] = [
      'target_id' => $floor['floor_id'],
    ];
    for ($i = 0; $i < 4; $i++) {
      $entity_data['name'] = $flat_number;
      $flat_number++;
      $entity_data['status'] = rand(1, 3);
  
      $flat = Flat::create($entity_data);
      $flat->save();
    }
  }
  

  The idea is to avoid "ladders" in your code. This adds an extra complexity during reading it and eventually, maintaining it.

• As a suggestion, change the definition way for a property (most of the places) for example:
  

    /**
     * The Presentation Plugin Manager service.
     */
    protected PresentationManager $presentationPluginManager;
    

  into:

    /**
     * The Presentation Plugin Manager service.
     *
     *  @var \Drupal\re_mgr_presentation\Manager\PresentationManager
     */
    protected PresentationManager $presentationPluginManager; 
    

• The twig file presentation-block.html.twig is missing the doxy definition. Check this link for more information https://www.drupal.org/docs/develop/coding-standards/twig-coding-standards →

Again, great job!

Thank You a lot @andrei.vesterli for deep review and good word! It's means to me a lot.

I analyzed your review and i made some fixes, also i have some questions:

• Readme is renamed and have updated syntax, thx for that tip i never saw anything telling about readme.
• You're right, i shouldn't support gin theme, so i removed related code with this.
• Purpose of re_mgr.api.php file is to inform developers that they can use this hook in their own modules if needed. It's common practice to keep information about all modules hooks in this place.
• Module is supporting PHP from 8.1 so it's compatible only with D10. It's very possible that it will run on PHP 7.4 even and D9, but it is a fresh module, not many people will use it on older D9 sites, also its not hard to upgrade to D10. Last is that i'm alone and i want to support only fresh D10.
• Thank You for code optimize tips, i included them on new module version. I love clean code so this will help me a lot in the future!
• I did not changed properties definitions because i really don't like this kind of syntax. Is this necessary to use it? If it's necessary please give me a link about this because i'm confused why to use them if we have typed properties?
• I added docs to the presentation block twig file.

Hi @Mattixonix

I am glad to help you! Regarding your comments/questions:

• Yes, README is something important in a module as it is a part of the documentation but you know, is always good to have decent and relevant documentation so, more people will understand the usage of your project
• Regarding the .api.php file, yes it has a purpose, and the purpose is to indicate that your module has some hooks definition and custom API. If it doesn't, you don't need to add it. Same thing for the .install file, if you don't have any defined hooks like _update_N, you don't need that file.
• Regarding the PHP 7.4, check this https://www.drupal.org/docs/getting-started/system-requirements/php-requ... → for more information. There is no longer support for D9+ for PHP 7.4, also, you can set a requirement for your module inside the composer.json file to set PHP 8.1+ for installation. So, no worries here.
• Glad to help!
• Regarding the usage for the doxy in Drupal, see this doc for more reference: https://www.drupal.org/node/1354#var → . You'll find many interesting things regarding doxy usage.
• Thx a lot!

Regards,
Andrei

The re_mgr.api.php file is fine. The example hook does not contain code, but that is not strictly necessary.
Drupal core sometimes shows code that just sets some array values, when there are not Drupal core implementations of the described hook. IMO, that code is not much helpful, as everybody should know how to set an array key.

/**
 * @file
 * Hooks and documentation related to Real Estate Manager.
 */

/**
 * Act on entity_bundle_after_create().
 *
 * This hook is invoked after entity bundle, form & view display save.
 *
 * @param string $entity_type_id
 *   The type of $entity; e.g. 're_mgr_building' or 're_mgr_flat'.
 */
function hook_entity_bundle_after_create($entity_type_id) {
  // When a new bundle is created, modules can do actions after
  // form & view display is saved, on it's configuration.
}

Thank all of You for your reviews and helpful tips, i'll definitely use them in the future! :)

I'm still wondering how many more reviews I need or how much time it will take to process this application?

I read the entries about Bonus program, but i see that there is no many active applications and i have reviews here, so i don't know is this necessary?

The documentation comments for class methods need to be updated and give a description for parameters and return values. The verb used in the description must be declined on the third person singular.
The documentation comments for hooks need to just contain the Implements hook_entity_operation(). line. The long description is not necessary, when it just repeats what the short description says, such as in the case of Add module related entities operations. A long description like Add library with icon to toolbar. is better to be moved in a comment inside the function code, as that explains what a single code line does.

Thank you for your contribution! I am going to update your account.

These are some recommended readings to help with excellent maintainership:

• Dries → ' post on Responsible maintainers
• Best practices for creating and maintaining projects →
• Maintaining a drupal.org project with Git →
• Commit messages - providing history and credit →
• Release naming conventions → .
• Helping maintainers in the issue queues →

You can find more contributors chatting on the Slack → #contribute channel. So, come hang out and stay involved → .
Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review → . I encourage you to learn more about that process and join the group of reviewers.

I thank all the reviewers.

Thanks again for review. I've extended all function and properties descriptions. I'm not a big fan of this way, because everything is typed so it's very clear what types i am using, still this is the rules and they are better then chaos ;)

About hooks, i did not removed longer description because as You said they are not mandatory and i really like when i see from the description what is going on right there not only knowing what hook is implemented. Also i know that i can comment inside function on specific line but rest of lines usually are used for this purpose mention in function description so i stay with this way.

Thank You for updating my account and all reviewers cause i learned a lot from this!

Automatically closed - issue fixed for 2 weeks with no activity.