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

Namespacing Composer extra configs #1387

Closed
nevvermind opened this issue Jun 18, 2015 · 10 comments
Closed

Namespacing Composer extra configs #1387

nevvermind opened this issue Jun 18, 2015 · 10 comments

Comments

@nevvermind
Copy link
Contributor

As Composer packages get used more and more, if not careful, some of their extra configuration may clash at some point in the future.

Not only that, but having an en-gross extra object, it won't be easy to figure out which config belongs to what package.

It would be great if Magento 2 would spearhead good practice (in my opinion at least) and use namespaced configs. We already have the best candidate for this: the package vendor + name combo - magento/magento2ce:

"extra": {
    "magento": {
        "magento2ce": {
            "component_paths": {
                "package-specific": "stuff"
            }    
        }
    }
}

If not good practice, at least M2 will shield itself from possible conflicts with other package configs.

This seems like a small change anyway. But with lots of benefits from where I'm standing.

@buskamuza
Copy link
Contributor

Hi @nevvermind , good idea. I'd use "magento/magento2", not "magento2ce" because CE is just a combination of Magento 2 components, but the logic is applicable for any M2 component (even, if it's not in CE).

Another concern here is that this component_paths is introduced by the original Hackathon Composer Installer for M1 and we based M2 installer on it. Changing the path now may confuse the ones, who already used the Hackathon installer. Also, for anybody who already implemented extensions for M2, this part must be changed to be compatible with the installer. Which sounds risky change. Still not so risky now, as after stable release.

Also, in general we try to eliminate usage of such installer at all and make components work from vendor folder directly (see #1013, #1206).

To conclude: it's a good idea for the base implementation of the installer, but I'm not sure that it will give much benefit now, when it's already in use. But thanks for the proposal, maybe we'll hear other thoughts about this here.

@nevvermind
Copy link
Contributor Author

I'd use "magento/magento2", not "magento2ce" because CE is just a combination of Magento 2 components, but the logic is applicable for any M2 component (even, if it's not in CE).

Yeah, I used "magento2ce" just to emphasize the <vendor>/<package> duo. But I still think it's better to use the package name exactly, just to stick to a standard, othewise it's back to people using whatever they want.

Another concern here is that this component_paths is introduced by the original Hackathon Composer

I didn't know. I assumed it was yours. In this case, yeah, you shouldn't change that. You should change only what's Magento's.

@Vinai
Copy link
Contributor

Vinai commented Jun 22, 2015

👍 for breaking bc now during the beta. 👍👍👍 if breaking bc helps adoption of placing modules directly into vendor.

@joshdifabio
Copy link
Contributor

👍 for breaking bc now during the beta.

I agree. I believe that any third parties developing for Magento 2 at this relatively early stage will be expecting BC breaks.

@nevvermind
Copy link
Contributor Author

@Vinai @joshdifabio - not sure exactly what you mean by "BC break" here. You mean to change the installer's extra config to a namespaced one?

@Vinai
Copy link
Contributor

Vinai commented Jun 22, 2015

Yes, I think we are on the same page. In this case breaking BC means changing the composer.json extra configuration structure in a way that requires existing modules to be updated in order for them to continue to work.

@nevvermind
Copy link
Contributor Author

This doesn't require other modules to change. Does it? It's only Magento2's code that needs a slight adjument - https://github.com/magento/magento2/search?utf8=%E2%9C%93&q=component_paths. Where's the BC break?

Another concern here is that this component_paths is introduced by the original Hackathon Composer

I didn't know. I assumed it was yours. In this case, yeah, you shouldn't change that. You should change only what's Magento's.

I got a little confused, because the hackathon installer is a fork, so M2 can change it. And I think they should.

@buskamuza
Copy link
Contributor

@nevvermind , existing extensions will have to change "extra" section you described in the original post in their composer.json files.

@piotrekkaminski
Copy link
Contributor

@buskamuza was it ever done?

@vkorotun vkorotun removed the PS label Aug 4, 2016
@piotrekkaminski
Copy link
Contributor

Thank you for your submission.

We recently made some changes to the way we process GitHub submissions to more quickly identify and respond to core code issues.

Feature Requests and Improvements should now be submitted to the new Magento 2 Feature Requests and Improvements forum (see details here).

We are closing this GitHub ticket and have moved your request to the new forum.

magento-team pushed a commit that referenced this issue Aug 7, 2017
magento-engcom-team added a commit that referenced this issue Aug 26, 2020
…te' of undefined' appears in dev console if save Previewed image as a new View and open this View on another page #29639

 - Merge Pull Request #29639 from joweecaquicla/magento2:ASI-1387-uncaught-type-error-new-on-views
 - Merged commits:
   1. 154ebc0
   2. 61f5cbe
   3. e2e6b3e
   4. d24d07d
   5. a2f56ab
   6. 5ff347c
magento-engcom-team pushed a commit that referenced this issue Aug 26, 2020
…te' of undefined" appears in dev console if save Previewed image as a new View and open this View on another page #29639
magento-engcom-team added a commit that referenced this issue Aug 26, 2020
Accepted Community Pull Requests:
 - #29639: #1387: "Uncaught TypeError: Cannot read property 'complete' of undefined" appears in dev console if save Previewed image as a new View and open this View on another page (by @joweecaquicla)
 - #29632: #1760: Media Gallery Page opened successfully if "Enhanced Media Gallery" disabled (by @jmonteros422)
 - #26081: Fix #26080 (by @korostii)
 - #29269: Issue 25595 - multiple address virtual product error at checkout fixe… (by @ralbin)
 - #27696: Initialize inline translations module only when they are enabled (by @krzksz)
 - #29482: [MFTF] Add test coverage  for AdminAnalytics module (by @Usik2203)
 - #29353: Fix #29194 Update tier price div visibility (by @srsathish92)
 - #28163: Fix #13401 - Multi-Store: "Store View" sort order values are not reflected in front-end store-switcher (by @Bartlomiejsz)
 - #27691: fixed condition when can show password input. Fixed logic for pulling… (by @ProkopovVitaliy)


Fixed GitHub Issues:
 - #1760: Question: Object Manager objects' get methods return 1 (reported by @CoreyCole) has been fixed in #29632 by @jmonteros422 in 2.4-develop branch
   Related commits:
     1. afe499d
     2. 0994c44
     3. 5a70b81
     4. 2267021

 - #26080: [2.4-develop] Cannot instantiate Magento\Framework\MessageQueue\ConfigInterface if optional module Magento_MessageQueue is disabled (reported by @korostii) has been fixed in #26081 by @korostii in 2.4-develop branch
   Related commits:
     1. cfab782
     2. 7c263e8

 - #25595: Add all type of product into cart, start checkout with multiple address after giving multiple address, except download and virtual type product, remove all four type product from the cart then getting exception  (reported by @Khushbu-Webkul-QA) has been fixed in #29269 by @ralbin in 2.4-develop branch
   Related commits:
     1. 67a4be3
     2. 3a1df1d
     3. 603ef05
     4. 78f0780
     5. b566b80
     6. e40b1ca
     7. 090f06a
     8. 54c8ebc

 - #29553: [Issue] Initialize inline translations module only when they are enabled (reported by @m2-assistant[bot]) has been fixed in #27696 by @krzksz in 2.4-develop branch
   Related commits:
     1. 28d1af5
     2. b842083
     3. fe00511
     4. d64eaf4
     5. 1861c47
     6. d7305bc
     7. 52432e5
     8. d3c9a49
     9. 52cb062
     10. d2883ed
     11. 217e993
     12. 703add1
     13. cbc6cc0

 - #29500: [Issue] [MFTF] Add test coverage  for AdminAnalytics module (reported by @m2-assistant[bot]) has been fixed in #29482 by @Usik2203 in 2.4-develop branch
   Related commits:
     1. c9f9506
     2. db43cb8
     3. e713317
     4. f2d1a0c
     5. 3f2df55
     6. 80efa90
     7. 9b69f75

 - #29194: CSS for tier prices block still visible when there are no tier prices (reported by @posidonius) has been fixed in #29353 by @srsathish92 in 2.4-develop branch
   Related commits:
     1. 3d7298e

 - #13401: Multi-Store: "Store View" sort order values are not reflected in the admin or front-end store-switcher (reported by @duffner) has been fixed in #28163 by @Bartlomiejsz in 2.4-develop branch
   Related commits:
     1. 85b6d05
     2. 47bc47b
     3. e87199e
     4. 06a5ac4
     5. a78ddde

 - #26903: Guest visitor e-mail in checkout kicks off password field visibility (reported by @speedupmate) has been fixed in #27691 by @ProkopovVitaliy in 2.4-develop branch
   Related commits:
     1. a9ca0f6
     2. fc1a6c5
     3. c412681
     4. 42803c8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants