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

Base Schema.org Implementation #6

Closed
wants to merge 54 commits into from
Closed

Conversation

shazmasiddiqui
Copy link
Collaborator

Pull Request for Issue #5

@shazmasiddiqui shazmasiddiqui changed the base branch from 4.3-dev to schema-system-plugin-test August 10, 2022 04:55
*
* @since 4.0.0
*/
protected function updateSchemaForm($data)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "update" mean?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function will update the Schema form with prefilled data from database if article already has some data.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please document this in the method phpdoc

*
* @return boolean
*/
public function onSchemaBeforeSave(AbstractEvent $event)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to have special function names for listeners. Offer a default subscriber map to [onSchedmaBeforeSave => saveSchema] so the schema plugin can use it in directly in getSubscribedEvents()

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay

*
* @since 4.0.0
*/
protected function saveSchema(AbstractEvent $event, $form)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use a more descriptive name. An example: storeSchemaToStandardLocation() -- anything that tells the user our trait function saves the schema to the standard location.

*
* @since 4.0.0
*/
protected function addSchemaType(Form $form, $type)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add return type annotation

*
* @since 4.0.0
*/
protected function saveSchema($event)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add return type annotation

*
* @return boolean
*/
protected function cleanupIndividualSchema(Registry $schema)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is a @todo, please mention it in a code comment.

*
* @return boolean
*/
protected function changeDurationFormat(Registry $schema, $durationKeys)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A function name like normalizeDurationsToISO could be more indicative of its function.

*
* @return boolean
*/
protected function changeDurationFormat(Registry $schema, $durationKeys)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type annotation for second param and return.

*
* @return array
*/
protected function convertToArray(Registry $schema, $repeatableFields)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarify why we need this function and document an example of the transformation it's performing in the phpdoc comment.

*
* @return boolean
*/
public function onSchemaAfterSave(AbstractEvent $event)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You do not need the onXXX functions. Map the functions to events in getSubscribedEvents() instead.

}
}
}
$schema->toArray();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are returning the schema as an array?

@anuragteapot anuragteapot changed the base branch from dev to 4.3-dev October 8, 2022 16:55
@anuragteapot anuragteapot changed the base branch from 4.3-dev to dev October 8, 2022 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants