search

apostrophecms / apostrophe

A full-featured, open-source content management framework built with Node.js that empowers organizations by combining in-context editing and headless architecture in a full-stack JS environment.
https://apostrophecms.com
Other
4.36k stars 590 forks source link

No warning for piece types adding a field called "type" #1365

Closed abea closed 1 year ago

abea commented 6 years ago

Right now there's no warning for creating a piece type with new added field with name: 'type'. This is used by Apostrophe already and any data users attempt to store there will be overridden.

I attempted to solve this by adding it to the forbiddenFields array in doc-type-manager, but that was too far-reaching. There might need to be solution closer tied to apostrophe-pieces.

boutell commented 5 years ago

Hint: this could be checked for in apostrophe-pieces and apostrophe-widgets in afterConstruct, where self.schema should already be fully baked. It should not be checked for in the universal case in apostrophe-schemas because for pages, a type field is appropriate.

itsajay1029 commented 2 years ago

Hey @boutell can you assign me this issue so that I can get started working on it ?

boutell commented 2 years ago

Hi @itsajay1029, we generally don't use the assign feature, but you are welcome to work on it, and we'll let this comment stand as an indication that we're aware of your intention to do so.

sajdakabir commented 1 year ago

is this issue open?

boutell commented 1 year ago

@sajdakabir offhand it looks like this still has not been addressed, at least not in A3. It's easy to check by trying it.

If you're interested in creating a PR, you would need to put that check in the @apostrophecms/piece-type and @apostrophecms/widget-type modules (for A3). See earlier notes for where to put it in A2 if you're interested in A2 (maintained in the 2.0 branch).

Ayushparui commented 1 year ago

@sajdakabir offhand it looks like this still has not been addressed, at least not in A3. It's easy to check by trying it.

If you're interested in creating a PR, you would need to put that check in the @apostrophecms/piece-type and @apostrophecms/widget-type modules (for A3). See earlier notes for where to put it in A2 if you're interested in A2 (maintained in the 2.0 branch).

Hello @boutell I'm new to open source. This codebase is a little overwhelming to me. What I understood by the issue is it's not providing any warning for creating a piece with name: 'type'. Would you mind elaborating on this?

BoDonkey commented 1 year ago

@itsajay1029, @sajdakabir, and @Ayushparui - I'm not sure how much experience any of you have with open-source contribution or Apostrophe. I'm a DevRel at Apostrophe and if any of you are interested, I'm willing to help you work toward a solution. I'm not quite sure what the best way to work together would be. Maybe a Discord channel? Any suggestions or interest?

Ayushparui commented 1 year ago

@itsajay1029, @sajdakabir, and @Ayushparui - I'm not sure how much experience any of you have with open-source contribution or Apostrophe. I'm a DevRel at Apostrophe and if any of you are interested, I'm willing to help you work toward a solution. I'm not quite sure what the best way to work together would be. Maybe a Discord channel? Any suggestions or interest?

Hey @BoDonkey thanks for replying. Discord sounds good, but I guess slack might be better.

BoDonkey commented 1 year ago

@itsajay1029, @sajdakabir, and @Ayushparui - I'm not sure how much experience any of you have with open-source contribution or Apostrophe. I'm a DevRel at Apostrophe and if any of you are interested, I'm willing to help you work toward a solution. I'm not quite sure what the best way to work together would be. Maybe a Discord channel? Any suggestions or interest?

Hey @BoDonkey thanks for replying. Discord sounds good, but I guess slack might be better.

@Ayushparui Hmmm, I know I can easily add Discord channels. Not sure about adding to our company Slack. Why do you prefer Slack?

Ayushparui commented 1 year ago

@itsajay1029, @sajdakabir, and @Ayushparui - I'm not sure how much experience any of you have with open-source contribution or Apostrophe. I'm a DevRel at Apostrophe and if any of you are interested, I'm willing to help you work toward a solution. I'm not quite sure what the best way to work together would be. Maybe a Discord channel? Any suggestions or interest?

Hey @BoDonkey thanks for replying. Discord sounds good, but I guess slack might be better.

@Ayushparui Hmmm, I know I can easily add Discord channels. Not sure about adding to our company Slack. Why do you prefer Slack?

@BoDonkey We'll go with Discord, no problem.

BoDonkey commented 1 year ago

@Ayushparui @itsajay1029 @sajdakabir Here is a link to our Discord. Please join if you haven't already. I'll make a channel for us, and we can coordinate there. Thanks for your interest in Apostrophe! https://discord.com/invite/XkbRNq7 I named the channel "open-source-contributions"

ShlokJswl commented 1 year ago

Hello Can you please assign this issue to me?

BoDonkey commented 1 year ago

Hi @ShlokJswl, We generally don't use the assign feature, but you are welcome to work on it. Reach out if you need any advice.

BoDonkey commented 1 year ago

Initial Thoughts on This Issue

Overall, after a quick glance at the codebase, the fix for this isn't terribly complex. However, it does require some understanding of the overall Apostrophe ecosystem. This makes this issue a little more work than issues in some of our standalone repositories.

How to work on this issue

  1. Fork this repo into your own account
  2. Make changes to the code - since it is in your account, you could work directly on main, but wouldn't you rather get some Git practice by working on a branch, and then merging to your own main?
  3. Write one or more tests to make sure your feature works
  4. Make sure your code changes don't break any existing tests.
  5. Update the CHANGELOG.md file - since other people might contribute during the same sprint cycle, add your change log message under 'UNRELEASED', not a specific version. We will add the version number when everything is approved and published on npm.
  6. When you are done, return to this repository and create a PR to pull code from your fork. Read more about this here. Make sure to fill out the PR template as best as you can.
  7. Navigate to our Discord server here if you have already joined, or here if you need an invitation and post a message in the open-source contribution channel that you need a PR review.
  8. After (hopefully) a short amount of time, one of our team engineers will review your PR and potentially advise you about things they want to see changed.
  9. If you need to make changes, go back to your local fork, make changes, and contribute those back to the main repo. This will update the PR.
  10. When you are done with changes and want a re-review, ask in Discord once again.
  11. After your PR is accepted, celebrate!!! 🎉

Code suggestions for this issue

I'm going to be thinking about this for a3, but the approach for a2 would be similar.

The comments above give some clues about how to approach this issue. A good first step is to spin up a project to make sure you can replicate the issue. For example, make a custom field type where one of the "fields" has a property name of type.

Looking at the originally proposed solution that didn't work proposed by @abea (for a2) and @boutell's follow-up gives a place to begin thinking about this problem. If you look at the code of the @apostrophecms/doc-type you will find a method called composeSchema(). Within this method, there is a variable called forbiddenFields that is set to an array of field names. Looking just below the definition of this variable, we can see it is used to check each field name in the schema to make sure it isn't named something listed in the array and throw an error if it is. This is what we have to implement. BUT, our job is made more tricky because as @boutell pointed out "for pages, a type field is appropriate". So, we have to implement this type of screening for both the @apostrophecms/piece-type and @apostrophecms/widget-type.

Finally, please make use of Discord to ask questions. Try to answer the questions yourself using internet resources, but don't be afraid to ask questions on Discord about anything. We are here to help!

varunseth578 commented 1 year ago

i would like to contribute to this issue.

BoDonkey commented 1 year ago

Hi @varunseth5788, Sorry, but we don't use the assign feature. Typically we just ask people to announce in the issue comments that they are working on the problem. For this issue, I know that another community member had submitted a PR. There are some open issues in the random-words repo that you could try to address. Cheers, Bob

rakshitsinghh commented 1 year ago

i would like to contiribute to this issue , how do i start

rakshitsinghh commented 1 year ago

Hey @boutell can you assign me this issue so that I can get started working on it ?

hey i need a favour , can we talk

aniruddhapw commented 1 year ago

i would love to contribute

BoDonkey commented 1 year ago

Hi @rakshitsinghh and @aniruddhapw, Another community member has almost completed this issue. We are just struggling with the tests a little. There are issues for the apostrophecms/random-words repo that are good first issues that you could work on. Thanks!

ghost commented 1 year ago

Hello I am new to open source after spending hours looking for something to contribute i finally found this thing which is mostly on javascript and node i would love to contribute but the thing i am a bit of confused there is just so many new things git , github and other things its just so much. Maybe if someone can guide me for a little start. I know javascript and node i need some real world experience thats why i chose to contribute to open source

BoDonkey commented 1 year ago

Hi @umer-not-umer, As I said in the comment just above yours, this issue is essentially solved. I thought it had already been closed and I apologize. You can take a look at the apostrophecms/random-words repo for a good first issue. Thanks!