Problem/Motivation

I see that the module follows very old PHP standard practices, for example it doesn't follow the function return type hinting, among other features that need to be added

More information about Drupal's coding standards

Steps to reproduce

Look into the module's source code, it is using outdated PHP practices.

Proposed resolution

Upgrade the module follow recent standard practices for writing more robust PHP code.

Remaining tasks

N/A

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Issue fork media_entity_link-3581198

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

danrod created an issue. See original summary.

danrod’s picture

danrod
Location Ottawa
commented
Issue tags: +Novice
aayushpathak’s picture

aayushpathak commented

working on it

aayushpathak’s picture

aayushpathak commented
Status: Active » Needs review

aayushpathak opened merge request !12

vinodhini.e’s picture

vinodhini.e commented

Hi,

Tested this module on Drupal 11.2.5.

Ran PHPCS coding standards.
No issues were reported. Thanks.

daniel.j’s picture

daniel.j commented

Currently this project does not have it's own phpstan.neon file. The config that it is using in the gitlab pipeline is the base Drupal one, which is set to level 0 (https://git.drupalcode.org/project/gitlab_templates/-/raw/default-ref/as...).

I wonder if a phpstan.neon could be added with a standard level in order to ensure good php standards into the future.

d.fisher’s picture

d.fisher commented

Sounds sensible. Should we do that on MR !12 or in a separate issue?

danrod’s picture

danrod
Location Ottawa
commented

I suggest adding it in the same MR !12.

d.fisher’s picture

d.fisher commented
Status: Needs review » Needs work

Added phpstan.neon and there be issues:
https://git.drupalcode.org/project/media_entity_link/-/jobs/9591949

Setting this back to "needs work".

aayushpathak’s picture

aayushpathak commented

Hi,
Thank you for the review. I ran PHPStan locally and did not encounter any errors.
Output for reference:

[OK] No errors

It seems the CI pipeline may be using a different configuration or level.
Thanks!

d.fisher’s picture

d.fisher commented
Status: Needs work » Needs review

Nice. Marking this as "needs review" again now pipelines are passing.

d.fisher’s picture

d.fisher commented

@danrod and @daniel.j would either of you be able to review the merge request now that everything is passing and provide your feedback. In the meantime I will test this is all still working as expected on a fresh site install.

d.fisher’s picture

d.fisher commented

I am satisfied that the MR applies cleanly and the functionality is unaffected. But I'd prefer that someone else signs off on the quality of the PHP changes. They look OK to me but I spend more time in the frontend of Drupal than the backend so the latest and greatest best practices are not my area of expertise!

daniel.j’s picture

daniel.j commented
Status: Needs review » Reviewed & tested by the community

Looked over changes and they all look good to me. Linting and unit tests pass for me and pipeline. Checked that the pipeline is using the correct neon file.

d.fisher’s picture

d.fisher commented

Nice I'll get this one merged.

d.fisher’s picture

d.fisher commented
Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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

d.fisher’s picture

d.fisher commented

Just tagged a 2.0.6 release with this in it.