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

Add class map and generator script for PhpStorm introspection. #548

Merged
merged 3 commits into from Jun 4, 2019

Conversation

Projects
None yet
7 participants
@colinmollenhour
Copy link
Contributor

commented Oct 4, 2018

If you use PhpStorm (and who doesn't!) this will make it's introspection work much much better without any additional plugins. For example, it will now be able to discern that Mage::getModel('catalog/product') returns an instance of Mage_Catalog_Model_Product. Also does helpers, singletons, resource models and resource singletons.

@sreichel

This comment has been minimized.

Copy link
Collaborator

commented Oct 5, 2018

Nice.

Are there any dis/advantages to https://github.com/Vinai/phpstorm-magento-mapper?

@Flyingmana

This comment has been minimized.

Copy link
Member

commented Oct 5, 2018

generally liking it, but why not use/expect the usage of magerun? Or do we have anything in there which is better/different?

@colinmollenhour

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2018

Vinai's script actually looks a bit more thorough, e.g. it adds an event observer phpstorm_map_generator_extend_map for extensions and supports the Mage_Core_Model_Factory and friends. I haven't looked at magerun's code but assume it is similar.

This isn't better than the other solutions, just thought it would be a useful addition for all the users who aren't aware that the PhpStorm feature even exists or how to use it. I can understand arguments for leaving it out as well. All feedback welcome.

@sreichel

This comment has been minimized.

Copy link
Collaborator

commented Oct 9, 2018

It is definitly useful.

I use Vinai's and haven't tested your one or magrun ...

How about add three links to README?

@Ig0r-M

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

Thanks @colinmollenhour to show me that this actually existed.

@Flyingmana

This comment has been minimized.

Copy link
Member

commented Mar 17, 2019

is it ok to close this one and instead add a section to the website with tips,scripts and modules for improved IDE integration?

@colinmollenhour

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

No objection to closing, but I think the number of people who will read the website enough to find this information will be quite small.

@tmotyl

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

Usually I use the metadata generator provided in n98-magerun.
I'm ok with getting the script (or even the result of the script run - so the generated metadata files).

@colinmollenhour colinmollenhour force-pushed the colinmollenhour:phpstorm-introspection branch from cf6a928 to dc934f9 May 22, 2019

@colinmollenhour colinmollenhour changed the base branch from 1.9.3.x to 1.9.4.x May 22, 2019

@colinmollenhour colinmollenhour force-pushed the colinmollenhour:phpstorm-introspection branch from dc934f9 to d161b98 May 22, 2019

@colinmollenhour

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

I've made the following changes:

  • Removed my script
  • Generated new class map using Vinai's script
  • Move class map file to .phpstorm.meta.php/core.meta.php so that other files can be added alongside it. The docs mention that:

    In PhpStorm version 2016.2 and later, you can have multiple separate files stored in a folder named .phpstorm.meta.php.

  • Added .htaccess file to prevent the file from being accessed publicly.
  • Added section to README to note the presence of this directory and how the file is generated (in case anyone wants to update it) and links to the PhpStorm docs.
  • Rebased onto 1.9.4.x

What do you think now? This lets the user immediately benefit from the file being included in the repo, but also allows them to add other files in the same directory without having to modify the "core" file. Extensions could even add and maintain their own meta files that build on the core one and symlink them into this directory as a convention. E.g. .phpstorm.meta.php/Aoe_Scheduler.meta.php. Of course that would be optional and the user could just generate a file covering the entire project in a file like .phpstorm.meta.php/project.meta.php and still not have to conflict with the core.meta.php file to avoid maintaining merge conflicts and such.

@sreichel

This comment has been minimized.

Copy link
Collaborator

commented May 22, 2019

What do you think now?

Lets merge :)

Changed ...

@sreichel

This comment has been minimized.

Copy link
Collaborator

commented May 28, 2019

Supporting PHPStorm isnt part of this project, so i dismissed my review.

It also could (not tested) cause problems, if there are already meta files generated by another tool. I guess the files get megerd in alphabetic order and this could reset mapping to Mage_XYZ for rewritten classes ...

'tax/config' instanceof \FireGento_MageSetup_Model_Tax_Config,

is it ok to close this one and instead add a section to the website with tips,scripts and modules for improved IDE integration?

Finally agree to @Flyingmana

@colinmollenhour

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

I tested the behavior with multiple files and here are the results:

  • .phpstorm.meta.php/abc.meta.php - core.meta.php wins
  • .phpstorm.meta.php/project.meta.php - project.meta.php wins
  • var/.phpstorm.meta.php - var/.phpstorm.meta.php wins

So, it is easy for the user to control which meta file is effective by choosing the proper file name or location and in all likelihood any existing files will probably take precedence over the new file. I don't see any harm in including this file, only massive benefit for those that do not know about this feature or are too lazy to implement a solution.

@sreichel

This comment has been minimized.

Copy link
Collaborator

commented May 30, 2019

only massive benefit for those that do not know about this feature or are too lazy to implement a solution

README.md? If they are to lazy ... ;)

Btw ... Vinai's script doesn't recognize disabled modules, like Mage_Backup - that has been disabled in 1.9.4. So code inspection isn't complete. N98-magerun seem to check all files ...

I'd let the users decide ... update README and point to Vinais script, N98 and maybe magicento.com?

@Flyingmana

This comment has been minimized.

Copy link
Member

commented May 30, 2019

to cover disabled modules is not a problem I think. Its nowhere I know common to "not show" auto-completion for unused code.

Having the generated files in is a different approach.
I see the direct benefit here in less setup work for a new project, and better support for developers who are still new and dont know about much of the tooling surrounding OpenMage/Magento1

I dont see a direct downside.

There is a risk of it getting out of sync with the repository. But we do not have enough change yet for this to matter.
In a followUp PR we could add a CI check for if the file needs an update, or even a github action to do this automatically.
Or we just put it on the release preparation List of Tasks (which we do not have yet, as we do not do releases yet)

What I wonder, is this using a PHP7.3 only language construct? the syntax check is currently failing everywhere except in 7.3.
Would suggest to exclude this file/directory from the check before merging

@Flyingmana
Copy link
Member

left a comment

I approve, but wish the travisCI Filecheck (Syntaxcheck of all php files) is changed accordingly.

@colinmollenhour

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

The syntax is not correct PHP syntax so it should just be ignored. I just pushed an update to .travis.yml which should fix.

@sreichel

This comment has been minimized.

Copy link
Collaborator

commented May 31, 2019

to cover disabled modules is not a problem I think. Its nowhere I know common to "not show" auto-completion for unused code.

I would not mention it, if i had not faced it (as a problem) ... ;)

Why shoud IDE support should be related to application stage?

I'd love to have "something" like this merged, as said above, but it shouldn't be the first solution. I've just tested N98 and it includes all classes and puts them into mentioned subfolder.

I'd approve, if we point to N98 (phar) instead and use their generated files.

Btw .. i'd not recommend to install magerun via composer.

@tbaden tbaden added the Review Needed label Jun 1, 2019

@sreichel sreichel removed the Review Needed label Jun 2, 2019

@sreichel
Copy link
Collaborator

left a comment

@colinmollenhour i'd love to see this feature in, but can you please use N98-magerun generated files and point there in readme?

Vinais script doent generate mapping for Mage::getResourceHelper() ... N98 does.

@colinmollenhour

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

I've updated to use n98-magerun for generating the class maps. Any final objections?

@sreichel sreichel requested a review from Flyingmana Jun 3, 2019

@Flyingmana Flyingmana merged commit 99da6e7 into OpenMage:1.9.4.x Jun 4, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sreichel

This comment has been minimized.

Copy link
Collaborator

commented Jun 7, 2019

@colinmollenhour are the files from EE?

Dont know if n98 accepts my PR ... if not, some additional files here: https://gist.github.com/sreichel/5515b518ce881bc41f5cb9eec6e55a9e

@colinmollenhour

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

The files were generated using the n98 phar downloaded using the README instructions and run against the latest magento-lts 1.9.4.x exactly as described in the updated README.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.