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

Adding namespaces for the Mailchecker class on the PHP Platform #417

Merged
merged 2 commits into from
Oct 18, 2023

Conversation

taimursalman
Copy link
Contributor

The following points pertain to MailChecker being used in PHP code.

As the library is being loaded via composer, the files get auto-loaded, and using require to load them again will be an unnecessary step. This PR aims to use namespaces instead. Some advantages of using namespaces over require statements are

  1. Namespace Isolation: Namespaces provide a way to isolate your code from the user's code, reducing the chance of naming conflicts.
  2. Improved Readability: When users import classes or functions via namespaces, their code becomes more readable because they can see exactly where a particular class or function is coming from.
  3. Easy Updates: If you need to release a new version of your library with improvements or bug fixes, users can simply update their Composer dependencies, and the latest version of your library will be automatically used.

@dmouse
Copy link
Contributor

dmouse commented Sep 28, 2023

can you include the autoload definition in the composer.json file, this can replace the classmap.

thank you for your contribution!

"autoload": {
    "psr-4": {
      "Fgribreau\\": "platform/php"
    }
  },

Update:

This is a breakthrough in the library, if you include the namespace this is not compatible with the previous class definition

@FGRibreau
Copy link
Owner

This is a breakthrough in the library, if you include the namespace this is not compatible with the previous class definition

Exactly, I will have to release a new major version of mailchecker but indeed it makes sense.

@taimursalman like @dmouse said, could you please update your PR to include the autoload definition in the composer.json file?

@taimursalman
Copy link
Contributor Author

taimursalman commented Oct 10, 2023

Certainly! Will do

Edit: The commit has been added

@FGRibreau
Copy link
Owner

@dmouse what do you think of the PR? Good to go?

@dmouse
Copy link
Contributor

dmouse commented Oct 16, 2023

@FGRibreau after this I think is a good idea include some documentation about how to upgrade to the previous version to this new version with namespace.

Edit:
I send this other PR just to update the doc #419

@dmouse dmouse mentioned this pull request Oct 16, 2023
@FGRibreau FGRibreau merged commit 232a78b into FGRibreau:master Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants